Revert "Revert #1536" #3246
Revert "Revert #1536" #3246
Conversation
Formerly "Attach empty onclick listener to each node". This reverts commit 431155d.
Without this, transitioning from `<div onClick={null} />` to `<div />` triggered `willDeleteListener` to delete the `click` handler which caused problems; now, we only call `putListener` and `deleteListener` when we have an actual listener.
I now also clean up the `onClickListeners` map upon deletion and don't double-listen when updating the event listener.
|
@yungsters Mind reviewing? You originally reviewed #1536 – this reverts your revert of that and adds a second commit on top to fix the issue it had, which was that when transitioning from I tested the original case that reproed the bug (t4444220) and it works correctly after this second commit. |
| var bankForRegistrationName = listenerBank[registrationName]; | ||
| // TODO: This should never be null -- when is it? |
There was a problem hiding this comment.
This should never be null -- when is it?
Hmm... when onClick={null} like you're trying to guard against in ReactDOMComponent.js?
What if instead of checking lastProps[propKey] in ReactDOMComponent.js, you only invoked PluginModule.willDeleteListener if !!bankForRegistrationName?
There was a problem hiding this comment.
You mean, if bankForRegistrationName[id]? I had that originally but it seemed like willDeleteListener should also be called if you switch from a function to null, which means that EventPluginHub.putListener probably needs to delegate to EventPluginHub.deleteListener if the listener is null… but the putListener function in ReactDOMComponent also does some enqueueing so now we're having listener deletion happen at a different time depending on whether it's gone completely from props or if it's just falsy…
It seemed easier to make ReactDOMComponent call putListener/deleteListener only when an actual listener is present and have EventPluginHub not worry about falsy values.
There was a problem hiding this comment.
Ahh, I see. Agreed. Do you think we should add a warning to deleteListener, then?
Either way, this looks good to me.
There was a problem hiding this comment.
Maybe. I didn't really change this though, just added the TODO. Will probably merge as-is and then maybe add the warning later.
I accidentally regressed this in facebook#3246. Now this matches what we already checked for updates.
Formerly "Attach empty onclick listener to each node" (#1536). This reverts commit 431155d and adds a fix on top for the bug I had.
Fixes #1169.