Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReactDOM - global event handler for a specific event type isn't removed once all the components with such event type handler are no longer mount #14587

Closed
domshlak opened this issue Jan 14, 2019 · 6 comments

Comments

@domshlak
Copy link

When a component with an event handler of a specific type (e.g. onClick) is mount, react dom adds a global event listener for this type.
The problem is that once all such components are no longer mount, the global event listener isn't removed.

The biggest problem that I see here is with the onMouseMove and onTouchMove event handlers.
For example, let's say that at some point there were mount components that had listeners for these event types, and later on all these components were unmounted (which means that there aren't any actual handlers for these event types).
At this point, react will keep tracking these events on the root, and each time that the pointer will move over the root, react will go over all the elements starting from the event target element up to the root element and try to see if there are handlers for these event types (though they don't actually exist).

The result is a potential performance issue, especially in mobile, since it seems that the global event handler isn't marked and can't be marked as a passive event.

@aweary
Copy link
Contributor

aweary commented Jan 14, 2019

Hey @domshlak! This has been brought up before, see #7128. It was decided that a better solution to this would be attaching listeners at the root DOM node of the application, rather than at the document. That task is being tracked in #2043.

Thanks!

@aweary aweary closed this as completed Jan 14, 2019
@domshlak
Copy link
Author

Hi @aweary,
attaching events on the root DOM node of the application won't solve this problem.
Doing that will just move the obsolete listener from the document to the application root.
The problem wasn't correctly identified in Issue #7128, since the listeners shouldn't be cleaned up after unmounting all roots, but to be cleaned up once the rendered components tree doesn't contain components with such listeners.

Consider the following example:

render(App, '#app-root');

class App extends React.Component {

   state = {
      showElementWithListeners: true
   };

   hideElementWithListeners = () => {
      this.setState({showElementWithListeners: false});  
   }
   
   doSomething = () => {
      
   };
   
   render () {
      
      return (
         <div>
            {
               this.state.showElementWithListeners ? 
                  (
                     <div
                        onClick={this.hideElementWithListeners}
                        onMouseDown={this.doSomething}
                        onMouseMove={this.doSomething}
                        onTouchMove={this.doSomething}
                     >
                        Element With Listeners
                     </div>
                  ) :
                  (
                     <div>
                        Element Without Listeners
                     </div>
                  )
            }            
         </div>
      )
      
   }
}

After clicking the element Element With Listeners there won't be any element with listeners for the click, mousedown, mousemove, touchmove events, but there will be a listener on the application.

@aweary
Copy link
Contributor

aweary commented Jan 14, 2019

Yeah, it's a slightly different issue but the answer is essentially the same. This only really becomes a problem if you're creating and destroying a bunch of React roots all the time.

If we were to remove unused event listeners, we'd have to re-add them when a component needs them again. The cost of doing this, along with the bookkeeping required to track which listeners are needed, is likely higher than the cost of just leaving the event listener registered.

In any case, it's never been reported as a performance bottleneck. If you have an example where this is causing noticeable performance issues, please feel free to share it!

Passive events are also something we've considered (#6436) and hope to implement someday, which would make this even less of a problem.

@domshlak
Copy link
Author

On the contrary, if the event listener will be attached on the application root as planned, it will become a problem only when you will not create and destroy a react root.
If a single root is there, then any attached event listener to it will remain there forever...

When you use react router for instance, the root remains the same, but sub views are replaced as a result of some sort of navigation. Once a view with an element that has onMouseDown callback attached to it, will be rendered, there will be a mouse down listener on the app root forever...
Since the listener won't be passive, all the scrolling interactions initiation in mobile will start only after this handler is executed.
In addition, even if passive event listeners will be supported, applications that will contain at some point a non passive listener (which might be required in some situations) will get the same performance penalty just because it contained a some point a component with a non passive event...

@kyletheninja
Copy link

I would suggest making it optional setting or even just a flag at the beginning of your meta listener, ie

if (listenerCount) {
// skip trying to walk up from the target looking for listeners, since none exist
}

I would see why you wouldn't want to go through function initialization a bunch of extra times in order to solve for an uncommon use case, but two lines to increment/decrement a counter on registration/deregistration, and one boolean check on event dispatching shouldn't ever have any effect on performance, whereas on a slow device, a nontrivial operation like walking through the tree on every mouseevent could probably have a noticeable effect.

All that being said if domshlak is the one raising this issue and he can't provide evidence of a noticeable performance impact, then even though his point makes sense in theory it may be a premature optimization to to solve for it.

I am not having issues with this haha. I actually just came here looking for info on how to do basic global event registration within the react event space (register events on app root which I can avoid triggering with stopPropagation, without using window and having stopProp kill all react events)

@ArmorDarks
Copy link

In any case, it's never been reported as a performance bottleneck. If you have an example where this is causing noticeable performance issues, please feel free to share it!

Here's a case where it's impacting performance #21847

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants