Attach event per react container root, rather than on the document #2043
Comments
|
@chenglou That would help our situation with the editor a bit, yes. But what's more important for Atom I think is the ability to integrate React with HTML custom elements without requiring the use of the shadow DOM. Seems like #1711 would help that, and it seems like this idea might be necessary to achieve that as well? |
|
@nathansobo Can you clarify what you mean, "without requiring the use of the shadow DOM"? I'm a bit uninformed about how HTML custom elements work right now. |
Work in progress. `enterleave` plugin (and maybe `analyticsPlugin`) is broken because it relied on the old behavior. But wanted to put this out here for suggestions. There are also some comments that need to be changed, I'll do it when the code is finalized. If we attach the event listening/dispatching at container level, it'll benefit the case of `<Editor/><Plugin1/>` (both are container roots), since `Plugin1` won't disturb `Editor`. We also detach those listeners now. There wasn't really a need in the past. Fixes facebook#2043 Should help with facebook#1964
|
@syranide I don't see why we should close issues just because there's a PR open – what if we don't go with that particular implementation? We may still want to make the same fix. |
|
@spicyj Fair enough :) |
Current React implementation is attaching events to `document` level. This is breaking bubbling DOM behaviour not letting users to play properly with methods like `stopPropagtion`. Also attaching events to containers instead of `document` integrates better with other Javascript frameworks. With this change we start attaching events at container level. Fixes: facebook#2043 facebook#4335
Current React implementation is attaching events to `document` level. This is breaking bubbling DOM behaviour not letting users to play properly with methods like `stopPropagtion`. Also attaching events to containers instead of `document` integrates better with other Javascript frameworks. With this change we start attaching events at container level. Fixes: facebook#2043 facebook#4335
Current React implementation is attaching events to `document` level. This is breaking bubbling DOM behaviour not letting users to play properly with methods like `stopPropagtion`. Also attaching events to containers instead of `document` integrates better with other Javascript frameworks. With this change we start attaching events at container level. Fixes: facebook#2043 facebook#4335
Current React implementation is attaching events to `document` level. This is breaking bubbling DOM behaviour not letting users to play properly with methods like `stopPropagation`. Also attaching events to containers instead of `document` integrates better with other Javascript frameworks. With this change we start attaching events at container level. Fixes: facebook#2043 facebook#4335
|
As far as I can tell #8117 was the latest attempt to do this. It has requested changes (that weren't addressed). So if somebody wants to pick this up they'll probably want to read the discussion there. |
|
Just FYI, this effort may get new life due to: testing-library/react-testing-library#35 If I can't push this along myself, I'll try to encourage others to do so. Thanks! |
|
@trueadm has been working on this. Not yet 100% sure but it's likely we'll do this change in a major (right now it's behind a flag). |
@nathansobo will that help your event perf issues a bit? I'm not familiar with atom's plugin infrastructure, But this'll help if you have
<Editor/><Plugin1/>(tworenderComponents). Doesn't help if you have<Editor><Plugin1/></Editor>though, but I have some ideas to optimize events a bit more.This is nonetheless an ok idea, I think.
@spicyj
The text was updated successfully, but these errors were encountered: