From aa4d7ff2a39d8387dcdd03a0fcbd0764a300a5fd Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 18 Mar 2019 17:29:13 -0700 Subject: [PATCH] Deprecate module pattern (factory) components --- .../__tests__/ReactComponentLifeCycle-test.js | 11 ++++++++- .../__tests__/ReactCompositeComponent-test.js | 9 +++++++- .../ReactCompositeComponentState-test.js | 9 +++++++- .../ReactDOMServerIntegrationElements-test.js | 2 +- .../ReactErrorBoundaries-test.internal.js | 21 +++++++++++++---- ...eactLegacyErrorBoundaries-test.internal.js | 20 ++++++++++++---- packages/react-dom/src/__tests__/refs-test.js | 12 +++++++++- .../src/server/ReactPartialRenderer.js | 19 +++++++++++++++ .../src/ReactFiberBeginWork.js | 20 ++++++++++++++++ .../src/__tests__/ReactHooks-test.internal.js | 9 +++++++- ...eactHooksWithNoopRenderer-test.internal.js | 23 +++++++++++++------ .../ReactIncremental-test.internal.js | 11 +++++++-- ...tIncrementalErrorHandling-test.internal.js | 11 +++++++-- .../ReactStrictMode-test.internal.js | 15 +----------- 14 files changed, 151 insertions(+), 41 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js index 2bbd3c57c258f..05c70bac3ca96 100644 --- a/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js +++ b/packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js @@ -977,7 +977,16 @@ describe('ReactComponentLifeCycle', () => { }; const div = document.createElement('div'); - ReactDOM.render( c && log.push('ref')} />, div); + expect(() => + ReactDOM.render( c && log.push('ref')} />, div), + ).toWarnDev( + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Parent to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Parent.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, + ); ReactDOM.render( c && log.push('ref')} />, div); expect(log).toEqual([ diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js index de512b7ec1192..7766944ec6e7c 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponent-test.js @@ -118,7 +118,14 @@ describe('ReactCompositeComponent', () => { } const el = document.createElement('div'); - ReactDOM.render(, el); + expect(() => ReactDOM.render(, el)).toWarnDev( + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Child to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Child.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, + ); expect(el.textContent).toBe('test'); }); diff --git a/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js b/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js index f882dc01bef63..3c28f48337683 100644 --- a/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js +++ b/packages/react-dom/src/__tests__/ReactCompositeComponentState-test.js @@ -474,7 +474,14 @@ describe('ReactCompositeComponent-state', () => { } const el = document.createElement('div'); - ReactDOM.render(, el); + expect(() => ReactDOM.render(, el)).toWarnDev( + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Child to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Child.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, + ); expect(el.textContent).toBe('count:123'); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js index 33f63a26f2f97..535191d66e8a7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationElements-test.js @@ -644,7 +644,7 @@ describe('ReactDOMServerIntegration', () => { }, }; }; - checkFooDiv(await render()); + checkFooDiv(await render(, 1)); }); }); diff --git a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js index c982c787ee0b7..3b19a87ecbf87 100644 --- a/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js @@ -768,12 +768,23 @@ describe('ReactErrorBoundaries', () => { }; const container = document.createElement('div'); - ReactDOM.render( - - - , - container, + expect(() => + ReactDOM.render( + + + , + container, + ), + ).toWarnDev( + 'Warning: The component appears to be a function component that ' + + 'returns a class instance. ' + + 'Change BrokenComponentWillMountWithContext to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`BrokenComponentWillMountWithContext.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, ); + expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); }); diff --git a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js index 7cec1ac6e96fa..234558172fe7e 100644 --- a/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js @@ -742,11 +742,21 @@ describe('ReactLegacyErrorBoundaries', () => { }; const container = document.createElement('div'); - ReactDOM.render( - - - , - container, + expect(() => + ReactDOM.render( + + + , + container, + ), + ).toWarnDev( + 'Warning: The component appears to be a function component that ' + + 'returns a class instance. ' + + 'Change BrokenComponentWillMountWithContext to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`BrokenComponentWillMountWithContext.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, ); expect(container.firstChild.textContent).toBe('Caught an error: Hello.'); }); diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index f3ce43adb1778..7cc764bdefdac 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -166,7 +166,17 @@ describe('factory components', () => { }; } - const inst = ReactTestUtils.renderIntoDocument(); + let inst; + expect( + () => (inst = ReactTestUtils.renderIntoDocument()), + ).toWarnDev( + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Comp to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Comp.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, + ); expect(inst.refs.elemRef.tagName).toBe('DIV'); }); }); diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js index 142f932cf78f6..6a8231e56dfb6 100644 --- a/packages/react-dom/src/server/ReactPartialRenderer.js +++ b/packages/react-dom/src/server/ReactPartialRenderer.js @@ -179,6 +179,7 @@ let didWarnDefaultTextareaValue = false; let didWarnInvalidOptionChildren = false; const didWarnAboutNoopUpdateForComponent = {}; const didWarnAboutBadClass = {}; +const didWarnAboutModulePatternComponent = {}; const didWarnAboutDeprecatedWillMount = {}; const didWarnAboutUndefinedDerivedState = {}; const didWarnAboutUninitializedState = {}; @@ -525,6 +526,24 @@ function resolve( validateRenderResult(child, Component); return; } + + if (__DEV__) { + const componentName = getComponentName(Component) || 'Unknown'; + if (!didWarnAboutModulePatternComponent[componentName]) { + warningWithoutStack( + false, + 'The <%s /> component appears to be a function component that returns a class instance. ' + + 'Change %s to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + "`%s.prototype = React.Component.prototype`. Don't use an arrow function since it " + + 'cannot be called with `new` by React.', + componentName, + componentName, + componentName, + ); + didWarnAboutModulePatternComponent[componentName] = true; + } + } } inst.props = element.props; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index f1b65633324c6..6cc20994c266d 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -144,6 +144,7 @@ const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner; let didReceiveUpdate: boolean = false; let didWarnAboutBadClass; +let didWarnAboutModulePatternComponent; let didWarnAboutContextTypeOnFunctionComponent; let didWarnAboutGetDerivedStateOnFunctionComponent; let didWarnAboutFunctionRefs; @@ -151,6 +152,7 @@ export let didWarnAboutReassigningProps; if (__DEV__) { didWarnAboutBadClass = {}; + didWarnAboutModulePatternComponent = {}; didWarnAboutContextTypeOnFunctionComponent = {}; didWarnAboutGetDerivedStateOnFunctionComponent = {}; didWarnAboutFunctionRefs = {}; @@ -1222,6 +1224,24 @@ function mountIndeterminateComponent( typeof value.render === 'function' && value.$$typeof === undefined ) { + if (__DEV__) { + const componentName = getComponentName(Component) || 'Unknown'; + if (!didWarnAboutModulePatternComponent[componentName]) { + warningWithoutStack( + false, + 'The <%s /> component appears to be a function component that returns a class instance. ' + + 'Change %s to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + "`%s.prototype = React.Component.prototype`. Don't use an arrow function since it " + + 'cannot be called with `new` by React.', + componentName, + componentName, + componentName, + ); + didWarnAboutModulePatternComponent[componentName] = true; + } + } + // Proceed under the assumption that this is a class instance workInProgress.tag = ClassComponent; diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 8933125cbfdc9..455df5d557064 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1327,7 +1327,14 @@ describe('ReactHooks', () => { expect(renderCount).toBe(1); renderCount = 0; - renderer.update(); + expect(() => renderer.update()).toWarnDev( + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Factory to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Factory.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, + ); expect(renderCount).toBe(1); renderCount = 0; renderer.update(); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 6fd77a90e400c..a1e12ca463750 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -137,13 +137,22 @@ describe('ReactHooksWithNoopRenderer', () => { }; } ReactNoop.render(); - expect(Scheduler).toFlushAndThrow( - 'Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for' + - ' one of the following reasons:\n' + - '1. You might have mismatching versions of React and the renderer (such as React DOM)\n' + - '2. You might be breaking the Rules of Hooks\n' + - '3. You might have more than one copy of React in the same app\n' + - 'See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.', + expect(() => + expect(Scheduler).toFlushAndThrow( + 'Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen ' + + 'for one of the following reasons:\n' + + '1. You might have mismatching versions of React and the renderer (such as React DOM)\n' + + '2. You might be breaking the Rules of Hooks\n' + + '3. You might have more than one copy of React in the same app\n' + + 'See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.', + ), + ).toWarnDev( + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Counter to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Counter.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + {withoutStack: true}, ); // Confirm that a subsequent hook works properly. diff --git a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js index f94b8f52808af..9db2710e1a2a2 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncremental-test.internal.js @@ -2026,8 +2026,15 @@ describe('ReactIncremental', () => { ReactNoop.render(); expect(() => expect(Scheduler).toFlushWithoutYielding()).toWarnDev( - 'Legacy context API has been detected within a strict-mode tree: \n\n' + - 'Please update the following components: Recurse', + [ + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Recurse to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Recurse.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + 'Legacy context API has been detected within a strict-mode tree: \n\n' + + 'Please update the following components: Recurse', + ], {withoutStack: true}, ); expect(ops).toEqual([ diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js index 6979375d5a003..d1f360592b4b7 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js @@ -1596,8 +1596,15 @@ describe('ReactIncrementalErrorHandling', () => { expect(() => { expect(Scheduler).toFlushAndThrow('Oops!'); }).toWarnDev( - 'Legacy context API has been detected within a strict-mode tree: \n\n' + - 'Please update the following components: Provider', + [ + 'Warning: The component appears to be a function component that returns a class instance. ' + + 'Change Provider to a class that extends React.Component instead. ' + + "If you can't use a class try assigning the prototype on the function as a workaround. " + + '`Provider.prototype = React.Component.prototype`. ' + + "Don't use an arrow function since it cannot be called with `new` by React.", + 'Legacy context API has been detected within a strict-mode tree: \n\n' + + 'Please update the following components: Provider', + ], {withoutStack: true}, ); }); diff --git a/packages/react/src/__tests__/ReactStrictMode-test.internal.js b/packages/react/src/__tests__/ReactStrictMode-test.internal.js index fc279a50a2301..7c0b308e67cc3 100644 --- a/packages/react/src/__tests__/ReactStrictMode-test.internal.js +++ b/packages/react/src/__tests__/ReactStrictMode-test.internal.js @@ -835,7 +835,6 @@ describe('ReactStrictMode', () => {
-
); } @@ -845,14 +844,6 @@ describe('ReactStrictMode', () => { return null; } - function FactoryLegacyContextConsumer() { - return { - render() { - return null; - }, - }; - } - LegacyContextProvider.childContextTypes = { color: PropTypes.string, }; @@ -885,10 +876,6 @@ describe('ReactStrictMode', () => { color: PropTypes.string, }; - FactoryLegacyContextConsumer.contextTypes = { - color: PropTypes.string, - }; - let rendered; expect(() => { @@ -898,7 +885,7 @@ describe('ReactStrictMode', () => { '\n in StrictMode (at **)' + '\n in div (at **)' + '\n in Root (at **)' + - '\n\nPlease update the following components: FactoryLegacyContextConsumer, ' + + '\n\nPlease update the following components: ' + 'FunctionalLegacyContextConsumer, LegacyContextConsumer, LegacyContextProvider' + '\n\nLearn more about this warning here:' + '\nhttps://fb.me/react-strict-mode-warnings',