You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tapestry.apache.org by Ben Weidig <be...@netzgut.net> on 2022/05/25 06:45:20 UTC
@PublishEvent url lookup might fail
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.
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.
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>.
Workaround/Alternatives:
Either not using <t:container>, or putting all its descendant in another
<div>, so the parent lookup succeds.
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
Ben
Re: @PublishEvent url lookup might fail
Posted by Ben Weidig <be...@netzgut.net>.
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 <
thiagohp@gmail.com> wrote:
> On Wed, May 25, 2022 at 3:45 AM Ben Weidig <be...@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-unsubscribe@tapestry.apache.org
> For additional commands, e-mail: dev-help@tapestry.apache.org
>
>
Re: @PublishEvent url lookup might fail
Posted by "Thiago H. de Paula Figueiredo" <th...@gmail.com>.
On Wed, May 25, 2022 at 3:45 AM Ben Weidig <be...@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-unsubscribe@tapestry.apache.org
For additional commands, e-mail: dev-help@tapestry.apache.org