Hi Thiago, going through the code, it seems to be more complicated than I'd wished for, as usual with my features :-D
A quick proof-of-concept seems to work, but I don't like the implementation so far: https://gist.github.com/benweidig/a53023e0169efd5a2ffa60e5903e5ede The general problem/difficulty I see is how to effeciently pass the two types of events to the PublishServerSideEvents mixin. The mixin then has to do check two JSONArrays in the passed JSONObject, and might need to lookup <body>. I think, I'll create an issue for the idea and start with implementing the duplicate event check and more tests first, before adding "global" events. And maybe updating the documentation of the feature, to make it clearer why it might fail with <t:container> Cheers, Ben On Fri, May 27, 2022 at 9:24 PM Thiago H. de Paula Figueiredo < thiag...@gmail.com> wrote: > On Wed, May 25, 2022 at 3:45 AM Ben Weidig <b...@netzgut.net> wrote: > > > Hello! > > Hello! > > > I ran into an issue with @PublishEvent yesterday, not finding the correct > > tag with data-component-events attribute to look up the url. > > > > My scenario: > > > > The component uses <t:container /> and has multiple direct descendants, > > meaning it will be rendered as it has multiple root nodes. > > Therefore, the PublishServerSideEvents mixin renders the data attribute > on > > the first node. > > When I was writing that feature, I struggled to have all the cases I > had in mind covered and the data attribute would be placed in the > right place (i.e. the Java code to find where to put the > data-component-events attribute). In your case, it goes to the first > element generated by the component, which is a good guess in most > cases (i.e. component has a root element or component doesn't have a > root element but each event can be triggered in one place of the > generated HTML, which is why the search happens with preceding > siblings first before going through ancestors). Components without a > root element in their templates (i.e. all the cases in which > <t:container> is used correctly) are indeed the worst case scenario. > I'd avoid touching that code and consider having a root element as an > unofficial requirement. > > On the other hand, if you write a bunch of tests (something I admit I > didn't do) covering the current code, then I'm comfortable with the > changes you're suggesting (although, of course, me being comfortable > for a change is not a requirement for any commit on Tapestry). > > > If an event is triggered with t5/core/ajax from an element in another > > component node, the lookup will fall back to <body> to start looking. > > But our <body> has a data-component-events attribute from the layout > > component. So the lookup fails. > > > > > > My proposal: > > > > Adding "boolean global() default false" to @PublishEvent, which will add > > the events to <body> if true, regardless of the components position in > the > > DOM. > > The idea is that @PublishEvent events are component-bound events; that's > > why the lookup starts at the provided element, including recursively > going > > upwards. If no element is provided, it looks at the body anyway. > > But right now, there's no option to put the url to the event there from > > Java. > > I like the idea. > > > Possible risks: > > > > Duplicate event names on <body>. > > The mixin would need to verify this and throw an exception, or the global > > events are stored in another data-attribute like > > "data-global-component-events. > > In this case, the lookup logic must be adapted to look there first before > > falling back to <body>. > > I'd prefer treating duplicate event names as an error. It makes > everything simpler and easier to understand. Also, it's an event tied > to a component, so having the same event name associated with more > than one component would be very error-prone and confusing. > > > Workaround/Alternatives: > > > > Either not using <t:container>, or putting all its descendant in another > > <div>, so the parent lookup succeds. > > Also maybe refactor your component so the event isn't triggered in > more than one place? > > > Thoughts? > > I know it's an edge case, but I really like using <t:container> without > > further container-divs and many @PublishEvent throughout the project. > > If I find some time, I'm going to build a proof-of-concept over the > weekend. > > Cheers! > > > > > Cheers > > Ben > > > > -- > Thiago > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tapestry.apache.org > For additional commands, e-mail: dev-h...@tapestry.apache.org > >
