Scoped event handlers #2510
Scoped event handlers #2510
Conversation
WorldSEnder
added
breaking change
A-yew
Async event dispatching is surprisingly complicated. Make sure to see yewstack#2510 for details, comments and discussion
BundleRoot controls the element where listeners are registered. Current impl always uses the global registry on document.body
we have create_root and create_ssr
Async event dispatching is surprisingly complicated. Make sure to see yewstack#2510 for details, comments and discussion
WorldSEnder
force-pushed
the
scoped-events
branch
from
3025bf2
to
339f18d
Compare
Async event dispatching is surprisingly complicated. Make sure to see yewstack#2510 for details, comments and discussion
WorldSEnder
force-pushed
the
scoped-events
branch
from
339f18d
to
9d07ce4
Compare
|
Visit the preview URL for this PR (updated for commit e416162): https://yew-rs--pr2510-scoped-events-p27mqpv0.web.app (expires Thu, 31 Mar 2022 17:31:34 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
WorldSEnder
removed
the
S-waiting-on-author
|
@WorldSEnder can you please resolve the merge conflicts? I'll review it after once this is merge-able.
Does this also fix #2175? |
this should take care of catching original events in shadow doms
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Size Comparison
|
|
@hamza1311 done, and yes I've added documentation addressing this |
| state: self.state.clone(), | ||
| event: UpdateEvent::Shift(parent, next_sibling), | ||
| })) | ||
| let mut state_ref = self.state.borrow_mut(); |
There was a problem hiding this comment.
I think shifting should be processed as a scheduler runnable.
For a couple of reasons:
- It guarantees that only 1 component is mutably borrowed at a time and the component is available to be borrowed.
- It allows higher priority events to be processed before it.
(the component can destroy itself before a shift if both events present) - It allows code that calls DOM APIs to be committed at smaller chunks by simply modifying the scheduler (future optimisation).
There was a problem hiding this comment.
I changed this for the simple reason that shifting is and should not be part of the lifecycle. It's also slightly easier not to have to capture the subtree.
It guarantees that only 1 component is mutably borrowed at a time and the component is available to be borrowed.
This should only be relevant if the component is nested in itself, which should not happen.
It allows higher priority events to be processed before it.
Given that other nodes are shifted immediately, delaying the shifting just messes up intermediate dom state. Think of a list of mixed components and normal elements that gets shifted. Delaying will (for some time) keep the components somewhere else than the elements.
Recipe for disaster, imo, and for event handling, finding the current correct parent needs to work on a correct hierarchy in the DOM. If the parent of a component is outdated, who knows what exactly happens, but it's just harder to remind everyone not to process events while components still have to be shifted. I don't see a benefit to delaying the shift. Again, a shift should only need to have to touch a handful, or even just one, node - all siblings at the top-level.
It allows code that calls DOM APIs to be committed at smaller chunks
I'm doubtful. Shifting an element should take a handful of calls, since it doesn't need to be done recursively, like rerendering. Once you drilled down to the root element, you just shift it and be done. The best candidate optimization I see is using the Range.extractContents API for lists, to move them in one chunk, but it doesn't work well with custom VRef(..) elements (the elements don't retain their identity, if I read the docs correctly. At least listeners are unregistered).
The real chunking should happen in view calls, which do work recursively and do require potentially a lot of calls. Just the amount of setAttribute calls and marshalling of strings accounts for ~30% of rendering costs in the benchmarks, if I remember numbers correctly.
There was a problem hiding this comment.
@futursolo I'll wait for your opinion on this, before I merge
There was a problem hiding this comment.
This should only be relevant if the component is nested in itself, which should not happen.
I think that mutably borrowing multiple components following the component tree downward is not a good idea.
As there're already other APIs relying on searching component states by immutably borrowing and searching the component tree upwards (e.g.: Scope::context()) and making sure them always not colliding seems a lot of work when working with APIs in this area.
Given that other nodes are shifted immediately, delaying the shifting just messes up intermediate dom state.
Rendering, attaching and destroying of components are all delayed as well.
Ultimately with #2396, every element will also be its own component which means every child element will be shifted at a later time as well.
Recipe for disaster, imo, and for event handling, finding the current correct parent needs to work on a correct hierarchy in the DOM.
Shifting into the detached parent of a Suspense should not be an issue as it's not possible to interact with elements inside the detached parent.
Should components use shifting when the Portal host changes?
IMHO, like it's not possible to change where a rendered application is mounted, I think Portal should be detached and re-attached if host changes. This should also allow to get rid of cache which simplifies the design.
(This is also how React handles Portal.)
Shifting an element should take a handful of calls,
There's nothing preventing an element from having 10,000 child elements.
Rendering is different than shifting, it will only result in the same number of API calls as the number of changes required for subsequent renders.
However, shifting 10,000 children will issue 10,000 DOM API calls every time shifting happens. If this can be split at component level, it means that we can split rendering into small chunks at component level (and element level with #2396) by simply modifying scheduler to commit a few runnables at a time.
There was a problem hiding this comment.
I think that mutably borrowing multiple components following the component tree downward is not a good idea.
As there're already other APIs relying on searching component states by immutably borrowing and searching the component tree upwards
Shifting doesn't use any of those APIs and I don't see a reason why that would change. There isn't and shouldn't be a way for user-code to run and do arbitrary things during shifting. That's the difference to "Rendering, attaching and destroying" which have user-defined code run. Shifts should be invisible, the user should only expect to control the tree downwards, etc. That's the argument of not being a lifecycle event.
Should components use shifting when the Portal host changes?
Maybe, but time for a future PR.
Shifting into the detached parent of a Suspense should not be an issue as it's not possible to interact with elements inside the detached parent.
Not directly, but events do run user-defined event handlers, which can run arbitrary code, so the DOM should be reasonably correct. I'm also not convinced that #2396 will actually switch to components under the hood (due to performance, creating a component has a noticeable overhead) and not just do the compile-time type checking. In any case, until that is decided, there'd be friction, should this PR be blocked until then? See below, 2396 is not relevant any time soon. So an event handler on an element, sibling to a component, could see inconsistent state if only the element is shifted and shifting of the component is delayed.
There's nothing preventing an element from having 10,000 child elements.
And most have a handful. Also, shifting 1e4 children would take the same time even if the lifecycle was delayed. You'd want to shift it all in one tick, lest be interrupted by other events and as said above, you shall not run user code in between. If there's truly a use-case for that many children, besides benchmarking, there are probably ways to correctly paginate and wrap those in a hierarchy (think of .git storing objects in folders labeling the first few characters of the object id, etc..) to limit the branching factor. And then you'd only have to shift the roots.
If the edge case works correctly, but has subpar performance, I don't think it's worth designing around it. Even more so if there's a reasonable work-around.
You'd want the shift of all those to take place in one tick, to prevent the browser from running pointless layout recomputations, anyway, as far as I am aware.
There was a problem hiding this comment.
I'm also not convinced that #2396 will actually switch to components under the hood
(due to performance, creating a component has a noticeable overhead) and not just do the compile-time type checking.
We should assume that PR will not be merged, at least not in the current state. The current implementation has bigger problems than performance overhead (the trade-off between hundreds of if lets/huge arrays). Elements will need to be special cased at library level but with static typing.
In any case, until that is decided, there'd be friction, should this PR be blocked until then?
No, not at all.
There was a problem hiding this comment.
until that is decided, there'd be friction
A disagreement in timing where components and elements are shifted out of order does not mean it creates issues. (Especially when the scheduler still works in a way that everything is will be run before yielding to the browser event loop which means an event listener cannot run before shifting finishes.)
Even if it does so, as long as Subtree is update at the same time as it's shifted, it would be fine?
Not directly, but events do run user-defined event handlers, which can run arbitrary code
Ultimately, this has something to do with the event handler behaviour.
(In React, Some events such as click will not be handled twice before a rendering is committed and thus preventing this issue. I am not sure how they implemented it though.)
See: facebook/react#20924 (comment)
You'd want the shift of all those to take place in one tick, to prevent the browser from running pointless layout recomputations, anyway, as far as I am aware.
You should keep the browser to be responsive whenever possible even if you are updating dom. React commits as much as 1 tick (in terms of refresh rate) worth of updates before letting browser to paint for this exact reason.
In addition, there's a difference between allowing user-defined logic to run and allowing browser to paint while updating dom. React uses a MessageChannel trick to make sure that as soon as the browser paints, it will be notified again to commit more changes.
If the edge case works correctly, but has subpar performance, I don't think it's worth designing around it.
If you want to keep this current design, I am fine to merge as-is and re-visit this at a future date when the situation changes.
There was a problem hiding this comment.
The linked issue seems relevant, but as far as I can tell, we're already implementing the expected behavior (just skimmed it though).
If you want to keep this current design, I am fine to merge as-is and re-visit this at a future date when the situation changes.
Alright, I'll merge then, feel free to ping me if a related issue comes up. I'm not especially experienced with concurrent mode, so if that changes anything in the future, I'd like a notification 👍
WorldSEnder
force-pushed
the
scoped-events
branch
from
0a5e692
to
e416162
Compare
Description
This addresses event handling in portals and scopes event handlers to the sub-tree controlled by each respective AppHandle, instead of installing them globally on
document.body. Events also bubble through portals, i.e. up the virtual dom instead of the physical one. The benefit here is that events iniframes and other out-of-document content that don't natively capture/bubble there can be caught and handled.Closes #2154
Closes #1564
Closes #2175
I think this is technically a breaking change, though I can't think of a reasonable (ab)use of the old, global, event handling style right now.
Still need to write test cases reproducing some issue of events in portals and checking they don't get handled twice, etc.Need to maybe add a few test cases here and there. We'll see what comes up.
Checklist
cargo make pr-flow