Warn early for non-functional event listeners (#10453)
* Add early warning for non-functional event listeners * Use functional listeners in ReactDOMComponent test To avoid triggering the non-functional event listener component * spy on console.error in non-function EventPluginHub test This should warn from ReactDOMFiberComponent, but we just ignore it here * Remove redundant check for listener * Add expectation for non-functional listener warning in EventPluginHub * Hoist listener typeof check in hot paths * Include stack addendum in non-functional listener warning * Make it pretty * Remove it.onnly from ReactDOMFiber test * Fix message * Update expected message * Change invariant message to match the new style * Fix remaining warning
Brandon Dail committed
Aug 23, 2017 at 07:38 UTC
34780da5c8d702daf84a46ba436a3c580b95a3cb
5 files changed
+57
-4
src/renderers/__tests__/EventPluginHub-test.js
+12
-1
@@ -16,22 +16,33 @@ jest.mock('isEventSupported');
16
describe('EventPluginHub', () => {
17
var React;
18
var ReactTestUtils;
19
+ var ReactDOMFeatureFlags;
20
21
beforeEach(() => {
22
jest.resetModules();
23
React = require('react');
24
ReactTestUtils = require('react-dom/test-utils');
25
+ ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
26
});
27
28
it('should prevent non-function listeners, at dispatch', () => {
29
+ spyOn(console, 'error');
30
var node = ReactTestUtils.renderIntoDocument(
31
<div onClick="not a function" />,
32
);
33
expect(function() {
34
ReactTestUtils.SimulateNative.click(node);
35
}).toThrowError(
33
- 'Expected onClick listener to be a function, instead got type string',
36
+ 'Expected `onClick` listener to be a function, instead got a value of `string` type.',
37
);
38
+ if (ReactDOMFeatureFlags.useFiber) {
39
+ expectDev(console.error.calls.count()).toBe(1);
40
+ expectDev(console.error.calls.argsFor(0)[0]).toContain(
41
+ 'Expected `onClick` listener to be a function, instead got a value of `string` type.',
42
+ );
43
+ } else {
44
+ expectDev(console.error.calls.count()).toBe(0);
45
+ }
46
});
47
48
it('should not prevent null listeners, at dispatch', () => {
src/renderers/dom/fiber/ReactDOMFiberComponent.js
+20
@@ -34,6 +34,7 @@ var setTextContent = require('setTextContent');
34
35
if (__DEV__) {
36
var warning = require('fbjs/lib/warning');
37
+ var {getCurrentFiberStackAddendum} = require('ReactDebugCurrentFiber');
38
var ReactDOMInvalidARIAHook = require('ReactDOMInvalidARIAHook');
39
var ReactDOMNullInputValuePropHook = require('ReactDOMNullInputValuePropHook');
40
var ReactDOMUnknownPropertyHook = require('ReactDOMUnknownPropertyHook');
@@ -118,6 +119,16 @@ if (__DEV__) {
119
warning(false, 'Extra attributes from the server: %s', names);
120
};
121
122
+ var warnForInvalidEventListener = function(registrationName, listener) {
123
+ warning(
124
+ false,
125
+ 'Expected `%s` listener to be a function, instead got a value of `%s` type.%s',
126
+ registrationName,
127
+ typeof listener,
128
+ getCurrentFiberStackAddendum(),
129
+ );
130
+ };
131
+
132
var testDocument;
133
// Parse the HTML and read it back to normalize the HTML string so that it
134
// can be used for comparison.
@@ -223,6 +234,9 @@ function setInitialDOMProperties(
234
// Noop
235
} else if (registrationNameModules.hasOwnProperty(propKey)) {
236
if (nextProp) {
237
+ if (__DEV__ && typeof nextProp !== 'function') {
238
+ warnForInvalidEventListener(propKey, nextProp);
239
+ }
240
ensureListeningTo(rootContainerElement, propKey);
241
}
242
} else if (isCustomComponentTag) {
@@ -695,6 +709,9 @@ var ReactDOMFiberComponent = {
709
} else if (registrationNameModules.hasOwnProperty(propKey)) {
710
if (nextProp) {
711
// We eagerly listen to this even though we haven't committed yet.
712
+ if (__DEV__ && typeof nextProp !== 'function') {
713
+ warnForInvalidEventListener(propKey, nextProp);
714
+ }
715
ensureListeningTo(rootContainerElement, propKey);
716
}
717
if (!updatePayload && lastProp !== nextProp) {
@@ -934,6 +951,9 @@ var ReactDOMFiberComponent = {
951
}
952
}
953
} else if (registrationNameModules.hasOwnProperty(propKey)) {
954
+ if (__DEV__ && typeof nextProp !== 'function') {
955
+ warnForInvalidEventListener(propKey, nextProp);
956
+ }
957
if (nextProp) {
958
ensureListeningTo(rootContainerElement, propKey);
959
}
src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
+22
@@ -18,6 +18,10 @@ var ReactTestUtils = require('react-dom/test-utils');
18
var PropTypes = require('prop-types');
19
20
describe('ReactDOMFiber', () => {
21
+ function normalizeCodeLocInfo(str) {
22
+ return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
23
+ }
24
+
25
var container;
26
var ReactFeatureFlags;
27
@@ -913,6 +917,24 @@ describe('ReactDOMFiber', () => {
917
]);
918
});
919
920
+ it('should warn for non-functional event listeners', () => {
921
+ spyOn(console, 'error');
922
+ class Example extends React.Component {
923
+ render() {
924
+ return <div onClick="woops" />;
925
+ }
926
+ }
927
+ ReactDOM.render(<Example />, container);
928
+ expectDev(console.error.calls.count()).toBe(1);
929
+ expectDev(
930
+ normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]),
931
+ ).toContain(
932
+ 'Expected `onClick` listener to be a function, instead got a value of `string` type.\n' +
933
+ ' in div (at **)\n' +
934
+ ' in Example (at **)',
935
+ );
936
+ });
937
+
938
it('should not update event handlers until commit', () => {
939
let ops = [];
940
const handlerA = () => ops.push('A');
src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
+2
-2
@@ -1690,8 +1690,8 @@ describe('ReactDOMComponent', () => {
1690
ReactTestUtils.renderIntoDocument(
1691
<div className="foo1">
1692
<div class="foo2" />
1693
- <div onClick="foo3" />
1694
- <div onclick="foo4" />
1693
+ <div onClick={() => {}} />
1694
+ <div onclick={() => {}} />
1695
<div className="foo5" />
1696
<div className="foo6" />
1697
</div>,
src/renderers/shared/shared/event/EventPluginHub.js
+1
-1
@@ -163,7 +163,7 @@ var EventPluginHub = {
163
164
invariant(
165
!listener || typeof listener === 'function',
166
- 'Expected %s listener to be a function, instead got type %s',
166
+ 'Expected `%s` listener to be a function, instead got a value of `%s` type.',
167
registrationName,
168
typeof listener,
169
);