Only call put/deleteListener with truthy listeners
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.
Ben Alpert committed
Feb 23, 2015 at 14:52 UTC
1943a9a0db859f2cbb7aac71079175a2f9368b86
4 files changed
+53
-4
src/browser/eventPlugins/SimpleEventPlugin.js
+8
-1
@@ -431,13 +431,20 @@ var SimpleEventPlugin = {
431
// listener on the target node.
432
if (registrationName === ON_CLICK_KEY) {
433
var node = ReactMount.getNode(id);
434
- onClickListeners[id] = EventListener.listen(node, 'click', emptyFunction);
434
+ if (!onClickListeners[id]) {
435
+ onClickListeners[id] = EventListener.listen(
436
+ node,
437
+ 'click',
438
+ emptyFunction
439
+ );
440
+ }
441
}
442
},
443
444
willDeleteListener: function(id, registrationName) {
445
if (registrationName === ON_CLICK_KEY) {
446
onClickListeners[id].remove();
447
+ delete onClickListeners[id];
448
}
449
}
450
src/browser/ui/ReactDOMComponent.js
+11
-2
@@ -351,7 +351,12 @@ ReactDOMComponent.Mixin = {
351
}
352
}
353
} else if (registrationNameModules.hasOwnProperty(propKey)) {
354
- deleteListener(this._rootNodeID, propKey);
354
+ if (lastProps[propKey]) {
355
+ // Only call deleteListener if there was a listener previously or
356
+ // else willDeleteListener gets called when there wasn't actually a
357
+ // listener (e.g., onClick={null})
358
+ deleteListener(this._rootNodeID, propKey);
359
+ }
360
} else if (
361
DOMProperty.isStandardName[propKey] ||
362
DOMProperty.isCustomAttribute(propKey)) {
@@ -395,7 +400,11 @@ ReactDOMComponent.Mixin = {
400
styleUpdates = nextProp;
401
}
402
} else if (registrationNameModules.hasOwnProperty(propKey)) {
398
- putListener(this._rootNodeID, propKey, nextProp, transaction);
403
+ if (nextProp) {
404
+ putListener(this._rootNodeID, propKey, nextProp, transaction);
405
+ } else if (lastProp) {
406
+ deleteListener(this._rootNodeID, propKey);
407
+ }
408
} else if (
409
DOMProperty.isStandardName[propKey] ||
410
DOMProperty.isCustomAttribute(propKey)) {
src/browser/ui/__tests__/ReactDOMComponent-test.js
+33
@@ -405,6 +405,39 @@ describe('ReactDOMComponent', function() {
405
406
expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);
407
});
408
+
409
+ it("should handle null and missing properly with event hooks", function() {
410
+ var React = require('React');
411
+ var SimpleEventPlugin = require('SimpleEventPlugin');
412
+
413
+ SimpleEventPlugin.didPutListener = mocks.getMockFunction();
414
+ SimpleEventPlugin.willDeleteListener = mocks.getMockFunction();
415
+ var container = document.createElement('div');
416
+
417
+ React.render(<div onClick={null} />, container);
418
+ expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(0);
419
+ expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(0);
420
+
421
+ React.render(<div onClick={() => 'apple'} />, container);
422
+ expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(1);
423
+ expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(0);
424
+
425
+ React.render(<div onClick={() => 'banana'} />, container);
426
+ expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
427
+ expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(0);
428
+
429
+ React.render(<div onClick={null} />, container);
430
+ expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
431
+ expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);
432
+
433
+ React.render(<div />, container);
434
+ expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
435
+ expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);
436
+
437
+ React.unmountComponentAtNode(container);
438
+ expect(SimpleEventPlugin.didPutListener.mock.calls.length).toBe(2);
439
+ expect(SimpleEventPlugin.willDeleteListener.mock.calls.length).toBe(1);
440
+ });
441
});
442
443
describe('updateComponent', function() {
src/event/EventPluginHub.js
+1
-1
@@ -147,7 +147,7 @@ var EventPluginHub = {
147
*/
148
putListener: function(id, registrationName, listener) {
149
invariant(
150
- !listener || typeof listener === 'function',
150
+ typeof listener === 'function',
151
'Expected %s listener to be a function, instead got type %s',
152
registrationName, typeof listener
153
);