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

Imperative API for installing DOM event handlers #284

Closed
andreypopp opened this issue Aug 21, 2013 · 9 comments
Closed

Imperative API for installing DOM event handlers #284

andreypopp opened this issue Aug 21, 2013 · 9 comments

Comments

@andreypopp
Copy link
Contributor

Currently React allows installing event handler on DOM events only in declarative fashion, via component attributes at the time of a component instantiation.

I think this is nice and allows automatically uninstall such handlers in case of a component disposal. But for implementing some "low-level" machinery I think it would be useful to allow handlers to be installed after the component instantiation at arbitrary moments. That would allow installing an event handler in a mixin's componentDidMount (with a corresponding uninstall of such event handler in componentWillUnmount) callback.

For example currently I use jQuery for trapping clicks on anchors and invoking HTML5 pushState API instead of letting a browser to reload a page:

  ...
  componentDidMount: function() {
    $(this.getDOMNode()).on('click.react-route', 'a', function(e) {
      e.preventDefault();
      // invoke HTML5 pushState API
    });
  },

  componentWillUnmount: function() {
    $(this.getDOMNode()).off('click.react-route');
  }
 ...

The problem with that is I'm not able to use goodies provided by React's event plugins. So what I want instead is to handle click events with React's event system and install such handler in componentDidMount callback so I can extract such functionality into a mixin. Something like:

  ...
  handleClick: function(e) {
    // check if event's target is an anchor and invoke pushState API
  },

  componentDidMount: function() {
    this.addEventListener(eventTypes.onClick, this.handleClick);
  },

  componentWillUnmount: function() {
    this.removeEventListener(eventTypes.onClick, this.handleClick);
  }
 ...

Though maybe addEventListener and removeEventListener names are to verbose...

Other examples include handling focus/blur and arrow key clicks to control focus inside a component, ...

@zpao
Copy link
Member

zpao commented Aug 21, 2013

This is interesting and definitely solves a real problem. I agree that it's awkward to mix event handling systems... I bet @jordwalke has some thoughts about this.

Is your thinking that add/removeEventListener would work on the top level component returned from render? Or all events of that type that occur for any component inside this one? You're jQuery code targets specific <a>s. Would you want the machinery to do that in React as well?

@andreypopp
Copy link
Contributor Author

Is your thinking that add/removeEventListener would work on the top level component returned from render? Or all events of that type that occur for any component inside this one?

I expect the same behaviour as if you attached event handler via a corresponding property — in this specific case via onClick property. If event bubbles then I'll be able to observe events of this types which occur for any component inside the specified component.

You're jQuery code targets specific <a>s. Would you want the machinery to do that in React as well?

That's interesting question. I guess I'll need to go through event bubbling path with my code and check if it contains an anchor — I think that's how jQuery implements it, am I right? Not sure though if React should do that for me.

@andreypopp
Copy link
Contributor Author

Any thoughts on this?

What do you think if we add two more functions to ReactEventEmitter, like putAdditionalListener and deleteAdditionalListener which would be exposed to ReactComponent?

Regarding implementation strategy — I see there are a lot of optimisations regarding reducing allocations during event dispatching and reducing time of reconciliation phase.

I think we can make CallbackRegistry to store listeners in an array for each event, component pair where the first element of each array would represent a "primary" event handler (set via virtual DOM returned from component's render() method) so setting/removing it would be a constant time still.

Accumulation through dispatch path would not require any additional allocations if we change event._dispatchListeners to an object with component IDs as keys and references to array of listeners for a component as values. The only overhead we can get is that executing dispatch listeners will require iteration but we can specialise for a common case where only "primiary" event handler is set (e.g. if (event._dispatchListeners[componentId].length == 1)).

What do you think? May I submit a pull request?

@zpao
Copy link
Member

zpao commented Sep 6, 2013

I think this is something @jordwalke or @yungsters will be best equipped to answer since they've worked on the whole events system the most. Unfortunately they are both on vacation so not around to comment. Until then, maybe somebody else could comment? @petehunt? @sebmarkbage?

@sebmarkbage
Copy link
Collaborator

@tomocchino has been thinking about a declarative way to do component level event listeners. You could imagine declaring your events on the component class or in mixins. Something like:

// warning: Not real API
eventHandlers: {
  root: {
    onClick: function() { ... }
  },
  someRef: {
    onClick: function() { ... }
  }
}

That would allow you to separate the event handlers into a mixin and also use more explicit event delegation. It can also avoid allocation during reconciliation. So I think that would solve your use case. This is not currently on the short term road map though.

Personally, I'm looking to get away from using mixins as much as possible. There's a lot of problems composing mixins that you don't get with composable components, which after all is one of the things that makes React great.

For example, to trap clicks on links you could wrap your components instead of using a mixin:

render: function() {
  return <LinkTrapper usePushState={true}><div><a /><a /></div><div><a /></div></LinkTrapper>;
}

Since the events bubble, link trapper can catch them with it's DOM node. Currently LinkTrapper must define an extra, unnecessary DOM node to attach it's event listeners to. We could potentially create a fake component that can retrieve event listeners without actually rendering to the DOM. Then the implementation of LinkTrapper would look something like this:

handleClick: function(e) {
  // check if event's target is an anchor and invoke pushState API
},
render: function() {
  return <FakeDOMNode onClick={this.handleClick}>{this.props.children}</FakeDOMNode>;
}

I don't see us exposing a public imperative API since that goes against the declarative/functional approach that gives us a lot of benefits. However, we may expose access to the ReactEventEmitter or something for special low-level components that needs to reach under the hood.

@elado
Copy link

elado commented May 11, 2016

This would be a great addition.

For example, if I want to handle triple click in a generic way, I have to wrap a component in another wrapper that would also output another element, like this <div>:

class TripleClickWrapper extends Component {
  render() {
    return <div onClick={this._onClick}>{this.props.children}</div>
  }

  _onClick = e => { /* counts clicks and handles timeouts etc and calls props.onTripleClick */ }
}

// somewhere else:

<TripleClickWrapper onTripleClick={::this._doSomething}>
  <SomeComponent />
</TripleClickWrapper>

I don't want to use React.cloneElement(React.Children.only(this.props.children)) and inject a new onClick to props, because contained component might already have that event.

Currently the only solution to bind new events without a new element and without altering the nested component is by using regular DOM events, which, as stated by @andreypopp, doesn't have all the goodies of React event system, something like that:

class TripleClickWrapper extends Component {
  componentDidMount() {
    // the DOM node actually refers whatever is in <SomeComponent>,
    // as this component doesn't have DOM nodes
    ReactDOM.findDOMNode(this).addEventListener('click', this._onClick, false)
  }

  componentWillUnmount() {
    ReactDOM.findDOMNode(this).removeEventListener('click', this._onClick)
  }

  render() {
    return React.Children.only(this.props.children)
  }

  _onClick = e => { /* counts clicks and handles timeouts etc and calls props.onTripleClick */ }
}

I'd imagine it as:

class TripleClickWrapper extends Component {
  componentDidMount() {
    this.addEvent('onClick', this._onClick) // will also auto-remove on unmount
  }

  render() {
    return React.Children.only(this.props.children)
  }

  _onClick = e => { /* counts clicks and handles timeouts etc and calls props.onTripleClick */ }
}

@uryyyyyyy
Copy link

I want it!
like flumpt(https://github.com/mizchi/flumpt)

@felipeochoa
Copy link

I would like to throw my support for this issue. Here are a couple of other use-cases I've run into:

  • Dropdown menu needs to listen for clicks outside itself to automatically close the menu
  • Click-and-drag functionality sometimes needs to listen for mousemove/mouseup document-wide (e.g., lasso select around elements if the user drags outside the container element)
  • Modal window handling keypress to close when the user hits ESC

I like the idea of fake event-only nodes that don't render DOM, but your proposal only handles the "listen for events in my children" use-case. If you added additional props like onDocumentMouseDown for listening globally, you could handle pretty much everything. You could even add a for prop that constrains your handling to those on a particular ref.

@gaearon
Copy link
Collaborator

gaearon commented Oct 1, 2017

Seems like this use case is very niche.

As noted above, a separate imperative API is unlikely. It seems like an intermediate component is the best solution for this use case.

The problems mentioned by @felipeochoa don’t seem related to this thread, so let’s track them in #285.

@gaearon gaearon closed this as completed Oct 1, 2017
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

7 participants