Skip to content

Commit

Permalink
Deprecate module pattern (factory) components (#15145)
Browse files Browse the repository at this point in the history
  • Loading branch information
sebmarkbage committed Mar 19, 2019
1 parent 55cc921 commit acd65db
Show file tree
Hide file tree
Showing 14 changed files with 151 additions and 41 deletions.
11 changes: 10 additions & 1 deletion packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
Expand Up @@ -977,7 +977,16 @@ describe('ReactComponentLifeCycle', () => {
};

const div = document.createElement('div');
ReactDOM.render(<Parent ref={c => c && log.push('ref')} />, div);
expect(() =>
ReactDOM.render(<Parent ref={c => c && log.push('ref')} />, div),
).toWarnDev(
'Warning: The <Parent /> 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(<Parent ref={c => c && log.push('ref')} />, div);

expect(log).toEqual([
Expand Down
Expand Up @@ -118,7 +118,14 @@ describe('ReactCompositeComponent', () => {
}

const el = document.createElement('div');
ReactDOM.render(<Child test="test" />, el);
expect(() => ReactDOM.render(<Child test="test" />, el)).toWarnDev(
'Warning: The <Child /> 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');
});
Expand Down
Expand Up @@ -474,7 +474,14 @@ describe('ReactCompositeComponent-state', () => {
}

const el = document.createElement('div');
ReactDOM.render(<Child />, el);
expect(() => ReactDOM.render(<Child />, el)).toWarnDev(
'Warning: The <Child /> 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');
});
Expand Down
Expand Up @@ -644,7 +644,7 @@ describe('ReactDOMServerIntegration', () => {
},
};
};
checkFooDiv(await render(<FactoryComponent />));
checkFooDiv(await render(<FactoryComponent />, 1));
});
});

Expand Down
Expand Up @@ -768,12 +768,23 @@ describe('ReactErrorBoundaries', () => {
};

const container = document.createElement('div');
ReactDOM.render(
<ErrorBoundary>
<BrokenComponentWillMountWithContext />
</ErrorBoundary>,
container,
expect(() =>
ReactDOM.render(
<ErrorBoundary>
<BrokenComponentWillMountWithContext />
</ErrorBoundary>,
container,
),
).toWarnDev(
'Warning: The <BrokenComponentWillMountWithContext /> 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.');
});

Expand Down
Expand Up @@ -742,11 +742,21 @@ describe('ReactLegacyErrorBoundaries', () => {
};

const container = document.createElement('div');
ReactDOM.render(
<ErrorBoundary>
<BrokenComponentWillMountWithContext />
</ErrorBoundary>,
container,
expect(() =>
ReactDOM.render(
<ErrorBoundary>
<BrokenComponentWillMountWithContext />
</ErrorBoundary>,
container,
),
).toWarnDev(
'Warning: The <BrokenComponentWillMountWithContext /> 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.');
});
Expand Down
12 changes: 11 additions & 1 deletion packages/react-dom/src/__tests__/refs-test.js
Expand Up @@ -166,7 +166,17 @@ describe('factory components', () => {
};
}

const inst = ReactTestUtils.renderIntoDocument(<Comp />);
let inst;
expect(
() => (inst = ReactTestUtils.renderIntoDocument(<Comp />)),
).toWarnDev(
'Warning: The <Comp /> 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');
});
});
Expand Down
19 changes: 19 additions & 0 deletions packages/react-dom/src/server/ReactPartialRenderer.js
Expand Up @@ -179,6 +179,7 @@ let didWarnDefaultTextareaValue = false;
let didWarnInvalidOptionChildren = false;
const didWarnAboutNoopUpdateForComponent = {};
const didWarnAboutBadClass = {};
const didWarnAboutModulePatternComponent = {};
const didWarnAboutDeprecatedWillMount = {};
const didWarnAboutUndefinedDerivedState = {};
const didWarnAboutUninitializedState = {};
Expand Down Expand Up @@ -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;
Expand Down
20 changes: 20 additions & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Expand Up @@ -144,13 +144,15 @@ const ReactCurrentOwner = ReactSharedInternals.ReactCurrentOwner;
let didReceiveUpdate: boolean = false;

let didWarnAboutBadClass;
let didWarnAboutModulePatternComponent;
let didWarnAboutContextTypeOnFunctionComponent;
let didWarnAboutGetDerivedStateOnFunctionComponent;
let didWarnAboutFunctionRefs;
export let didWarnAboutReassigningProps;

if (__DEV__) {
didWarnAboutBadClass = {};
didWarnAboutModulePatternComponent = {};
didWarnAboutContextTypeOnFunctionComponent = {};
didWarnAboutGetDerivedStateOnFunctionComponent = {};
didWarnAboutFunctionRefs = {};
Expand Down Expand Up @@ -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;

Expand Down
Expand Up @@ -1327,7 +1327,14 @@ describe('ReactHooks', () => {
expect(renderCount).toBe(1);

renderCount = 0;
renderer.update(<Factory />);
expect(() => renderer.update(<Factory />)).toWarnDev(
'Warning: The <Factory /> 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(<Factory />);
Expand Down
Expand Up @@ -137,13 +137,22 @@ describe('ReactHooksWithNoopRenderer', () => {
};
}
ReactNoop.render(<Counter />);
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 <Counter /> 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.
Expand Down
Expand Up @@ -2026,8 +2026,15 @@ describe('ReactIncremental', () => {

ReactNoop.render(<Recurse />);
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 <Recurse /> 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([
Expand Down
Expand Up @@ -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 <Provider /> 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},
);
});
Expand Down
15 changes: 1 addition & 14 deletions packages/react/src/__tests__/ReactStrictMode-test.internal.js
Expand Up @@ -835,7 +835,6 @@ describe('ReactStrictMode', () => {
<div>
<LegacyContextConsumer />
<FunctionalLegacyContextConsumer />
<FactoryLegacyContextConsumer />
</div>
);
}
Expand All @@ -845,14 +844,6 @@ describe('ReactStrictMode', () => {
return null;
}

function FactoryLegacyContextConsumer() {
return {
render() {
return null;
},
};
}

LegacyContextProvider.childContextTypes = {
color: PropTypes.string,
};
Expand Down Expand Up @@ -885,10 +876,6 @@ describe('ReactStrictMode', () => {
color: PropTypes.string,
};

FactoryLegacyContextConsumer.contextTypes = {
color: PropTypes.string,
};

let rendered;

expect(() => {
Expand All @@ -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',
Expand Down

0 comments on commit acd65db

Please sign in to comment.