Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate module pattern (factory) components #15145

Merged
merged 1 commit into from
Mar 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ describe('ReactDOMServerIntegration', () => {
},
};
};
checkFooDiv(await render(<FactoryComponent />));
checkFooDiv(await render(<FactoryComponent />, 1));
});
});

Expand Down
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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