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

RFC: Delegate/Pool DOM Event Listeners #284

Closed
dvdzkwsk opened this issue Jun 24, 2016 · 3 comments
Closed

RFC: Delegate/Pool DOM Event Listeners #284

dvdzkwsk opened this issue Jun 24, 2016 · 3 comments

Comments

@dvdzkwsk
Copy link
Member

dvdzkwsk commented Jun 24, 2016

Disclaimer:

This is just me thinking out loud in an effort to initiate some brainstorming. I am not proposing this to be considered/worked on until after V1 is released, since that is the main priority.

Proposal

For components that need to reach outside of React land and bind listeners on the real DOM, such as the Dropdown, I think we should consider limiting how many event handlers are attached to the document, especially when this can occur in frequent succession. Here's an example of what I'd like to discourage. This could be achieved by creating a facade on top of the core DOM API that would perform pooling/event delegation behind the scenes. Basically, a pub/sub system.

Potential benefits:

  1. Eliminates event listeners being repeatedly added to and removed from the document. This is one aspect React has thankfully eliminated from the front end developer's life: having to decide if and when to delegate events. Stardust could just listen to these common events and act as a pub/sub system for pooled handlers.

  2. Smarter event handlers could return an unbind function (or off method), to make cleanup easier:

// idea #1 =======================
const listenToKeydown = documentApi.on('click', () => { /* ... */ })
listenToKeydown() // or listenToKeydown.off()

// idea #2 =======================
// (could just be an extension of #1, too)
this._documentEvents = new eventApi.Pool('document') // could be global
this._documentEvents.on('click', () => { /* ... */ })
this._documentEvents.on('keydown', () => { /* ... */ })

// later, in unmount, perhaps
this._documentEvents.unbindAll()
  1. By piping all attach/detach calls through a single place, we can easily do performance profiling or diagnose cases where too many handlers exist or leaked event handlers do not get detached. This is possible without our own API wrapper, but it would help.

  2. Could improve event mock-ability in testing. This is debatable.

  3. If the API was made to be intentionally cumbersome, i.e. with long method names for binding (something done frequently in React core), it would help to discourage widespread usage, since we should be doing this as little as possible.

  4. Event pooling could be scoped to a component-level, to remove the chance for developer error in forgetting to unbind events.

  5. Solutions may already exist for this. Have not looked, yet.

Potential downsides:

  1. Haven't performed extensive profiling, so it may not actually be worth it, in terms of both performance and developer cost. Additionally, maybe the use case for this is so low that it's unnecessary.

  2. Could introduce bugs that wouldn't have occurred with direct bindings.

  3. Adds another level of indirection and is another thing for contributors to understand.

@levithomason
Copy link
Member

Agree on all counts including downsides. I lean toward the number 2 direction. Thanks for noting this here.

@levithomason
Copy link
Member

Linking #424 Search, which is essentially a Dropdown

Linking #440 (Comment) Modal, adding event listeners only when necessary but avoiding duplicates

@levithomason
Copy link
Member

This has merit and we'd like help here, but closing for housekeeping.

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

No branches or pull requests

2 participants