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.

Sign up for GitHub

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

Jump to bottom

tweak unsafe lifecycle warnings #15431

Closed
wants to merge 11 commits into from
Closed

tweak unsafe lifecycle warnings #15431

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c35e37a
mark react-events as private so we publish script skips it for now
Apr 3, 2019
dc605c1
Merge remote-tracking branch 'upstream/master'
Apr 3, 2019
a09bd0f
Merge remote-tracking branch 'upstream/master'
Apr 3, 2019
48ffcee
Merge remote-tracking branch 'upstream/master'
Apr 12, 2019
7cc1379
Merge remote-tracking branch 'upstream/master'
Apr 15, 2019
e86d051
Merge remote-tracking branch 'upstream/master'
Apr 16, 2019
e3b06c6
tweak unsafe lifecycle warnings to include codemod suggestion
Apr 17, 2019
d884ec3
stray comment
Apr 17, 2019
ea444d2
more tweaks, update more tests
Apr 18, 2019
0523909
move the codemod line to the end, add "Note that the warning will sti…
Apr 18, 2019
1ba1642
review stuff
Apr 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 57 additions & 26 deletions packages/react-dom/src/__tests__/ReactComponentLifeCycle-test.js
View file
Open in desktop
Expand Up @@ -709,9 +709,9 @@ describe('ReactComponentLifeCycle', () => {
);
}).toLowPriorityWarnDev(
[
'componentWillMount is deprecated',
'componentWillReceiveProps is deprecated',
'componentWillUpdate is deprecated',
'componentWillMount has been renamed',
'componentWillReceiveProps has been renamed',
'componentWillUpdate has been renamed',
],
{withoutStack: true},
);
Expand Down Expand Up @@ -748,9 +748,9 @@ describe('ReactComponentLifeCycle', () => {
);
}).toLowPriorityWarnDev(
[
'componentWillMount is deprecated',
'componentWillReceiveProps is deprecated',
'componentWillUpdate is deprecated',
'componentWillMount has been renamed',
'componentWillReceiveProps has been renamed',
'componentWillUpdate has been renamed',
],
{withoutStack: true},
);
Expand Down Expand Up @@ -815,7 +815,10 @@ describe('ReactComponentLifeCycle', () => {
{withoutStack: true},
);
}).toLowPriorityWarnDev(
['componentWillMount is deprecated', 'componentWillUpdate is deprecated'],
[
'componentWillMount has been renamed',
'componentWillUpdate has been renamed',
],
{withoutStack: true},
);

Expand Down Expand Up @@ -863,7 +866,7 @@ describe('ReactComponentLifeCycle', () => {
'https://fb.me/react-async-component-lifecycle-hooks',
{withoutStack: true},
);
}).toLowPriorityWarnDev(['componentWillMount is deprecated'], {
}).toLowPriorityWarnDev(['componentWillMount has been renamed'], {
withoutStack: true,
});

Expand All @@ -887,7 +890,7 @@ describe('ReactComponentLifeCycle', () => {
'https://fb.me/react-async-component-lifecycle-hooks',
{withoutStack: true},
);
}).toLowPriorityWarnDev(['componentWillReceiveProps is deprecated'], {
}).toLowPriorityWarnDev(['componentWillReceiveProps has been renamed'], {
withoutStack: true,
});
});
Expand Down Expand Up @@ -921,7 +924,10 @@ describe('ReactComponentLifeCycle', () => {
{withoutStack: true},
);
}).toLowPriorityWarnDev(
['componentWillMount is deprecated', 'componentWillUpdate is deprecated'],
[
'componentWillMount has been renamed',
'componentWillUpdate has been renamed',
],
{withoutStack: true},
);

Expand Down Expand Up @@ -967,7 +973,7 @@ describe('ReactComponentLifeCycle', () => {
'https://fb.me/react-async-component-lifecycle-hooks',
{withoutStack: true},
);
}).toLowPriorityWarnDev(['componentWillMount is deprecated'], {
}).toLowPriorityWarnDev(['componentWillMount has been renamed'], {
withoutStack: true,
});

Expand All @@ -990,7 +996,7 @@ describe('ReactComponentLifeCycle', () => {
'https://fb.me/react-async-component-lifecycle-hooks',
{withoutStack: true},
);
}).toLowPriorityWarnDev(['componentWillReceiveProps is deprecated'], {
}).toLowPriorityWarnDev(['componentWillReceiveProps has been renamed'], {
withoutStack: true,
});
});
Expand Down Expand Up @@ -1130,9 +1136,9 @@ describe('ReactComponentLifeCycle', () => {
ReactDOM.render(<MyComponent foo="bar" />, div),
).toLowPriorityWarnDev(
[
'componentWillMount is deprecated',
'componentWillReceiveProps is deprecated',
'componentWillUpdate is deprecated',
'componentWillMount has been renamed',
'componentWillReceiveProps has been renamed',
'componentWillUpdate has been renamed',
],
{withoutStack: true},
);
Expand Down Expand Up @@ -1403,17 +1409,42 @@ describe('ReactComponentLifeCycle', () => {
ReactDOM.render(<MyComponent x={1} />, container),
).toLowPriorityWarnDev(
[
'componentWillMount is deprecated and will be removed in the next major version. ' +
'Use componentDidMount instead. As a temporary workaround, ' +
'you can rename to UNSAFE_componentWillMount.' +
'\n\nPlease update the following components: MyComponent',
'componentWillReceiveProps is deprecated and will be removed in the next major version. ' +
'Use static getDerivedStateFromProps instead.' +
'\n\nPlease update the following components: MyComponent',
'componentWillUpdate is deprecated and will be removed in the next major version. ' +
'Use componentDidUpdate instead. As a temporary workaround, ' +
'you can rename to UNSAFE_componentWillUpdate.' +
'\n\nPlease update the following components: MyComponent',
'componentWillMount has been renamed to UNSAFE_componentWillMount, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
'- If you initialize state in componentWillMount, move this logic into the constructor.\n' +
'- If you fetch data or perform other side effects in componentWillMount, ' +
'move this logic into componentDidMount.\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: MyComponent\n',

'componentWillReceiveProps has been renamed to UNSAFE_componentWillReceiveProps, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
"- If you're updating state whenever props change, " +
'move this logic into static getDerivedStateFromProps.\n' +
'- If you fetch data or perform other side effects in componentWillReceiveProps, ' +
'move this logic into componentDidUpdate.\n' +
'- Refactor your code to use memoization techniques instead of derived state. ' +
'Learn more at: https://fb.me/react-derived-state\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: MyComponent\n',

'componentWillUpdate has been renamed to UNSAFE_componentWillUpdate, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
'- If you fetch data or perform other side effects in componentWillUpdate, ' +
'move this logic into componentDidUpdate.\n' +
"- If you're reading DOM properties before an update, " +
'move this logic into getSnapshotBeforeUpdate.\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: MyComponent\n',
],
{withoutStack: true},
);
Expand Down
17 changes: 7 additions & 10 deletions packages/react-dom/src/__tests__/ReactDOMServerLifecycles-test.js
View file
Open in desktop
Expand Up @@ -229,7 +229,7 @@ describe('ReactDOMServerLifecycles', () => {

expect(() =>
ReactDOMServer.renderToString(<Component />),
).toLowPriorityWarnDev('componentWillMount() is deprecated', {
).toLowPriorityWarnDev('componentWillMount has been renamed', {
withoutStack: true,
});
expect(log).toEqual(['componentWillMount', 'UNSAFE_componentWillMount']);
Expand Down Expand Up @@ -286,10 +286,9 @@ describe('ReactDOMServerLifecycles', () => {

expect(() =>
ReactDOMServer.renderToString(<Component />),
).toLowPriorityWarnDev(
'Component: componentWillMount() is deprecated and will be removed in the next major version.',
{withoutStack: true},
);
).toLowPriorityWarnDev('componentWillMount has been renamed', {
withoutStack: true,
});
});

it('should warn about deprecated lifecycle hooks', () => {
Expand All @@ -302,11 +301,9 @@ describe('ReactDOMServerLifecycles', () => {

expect(() =>
ReactDOMServer.renderToString(<Component />),
).toLowPriorityWarnDev(
'Warning: Component: componentWillMount() is deprecated and will be removed ' +
'in the next major version.',
{withoutStack: true},
);
).toLowPriorityWarnDev('componentWillMount has been renamed', {
withoutStack: true,
});

// De-duped
ReactDOMServer.renderToString(<Component />);
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/__tests__/ReactServerRenderingHydration-test.js
View file
Open in desktop
Expand Up @@ -363,7 +363,7 @@ describe('ReactDOMServerHydration', () => {
expect(() => {
element.innerHTML = ReactDOMServer.renderToString(markup);
}).toLowPriorityWarnDev(
['componentWillMount() is deprecated and will be removed'],
['componentWillMount has been renamed to UNSAFE_componentWillMount'],
{withoutStack: true},
);
expect(element.textContent).toBe('Hi');
Expand All @@ -373,7 +373,7 @@ describe('ReactDOMServerHydration', () => {
'Please update the following components to use componentDidMount instead: ComponentWithWarning',
);
}).toLowPriorityWarnDev(
['componentWillMount is deprecated and will be removed'],
['componentWillMount has been renamed to UNSAFE_componentWillMount'],
{withoutStack: true},
);
expect(element.textContent).toBe('Hi');
Expand Down
19 changes: 12 additions & 7 deletions packages/react-dom/src/server/ReactPartialRenderer.js
View file
Open in desktop
Expand Up @@ -573,13 +573,18 @@ function resolve(
if (!didWarnAboutDeprecatedWillMount[componentName]) {
lowPriorityWarning(
false,
'%s: componentWillMount() is deprecated and will be ' +
'removed in the next major version. Read about the motivations ' +
'behind this change: ' +
'https://fb.me/react-async-component-lifecycle-hooks' +
'\n\n' +
'As a temporary workaround, you can rename to ' +
'UNSAFE_componentWillMount instead.',
'componentWillMount has been renamed to UNSAFE_componentWillMount, ' +
"and the old name won't work in the next major version of React.\n" +
'We suggest doing one of the following:\n' +
'- If you initialize state in componentWillMount, move this logic into the constructor.\n' +
'- If you fetch data or perform other side effects in componentWillMount, ' +
'move this logic into componentDidMount.\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: %s\n' +
'\nLearn about this warning, with more examples and suggestions here:\n' +
'https://fb.me/react-async-component-lifecycle-hooks',
componentName,
);
didWarnAboutDeprecatedWillMount[componentName] = true;
Expand Down
57 changes: 40 additions & 17 deletions packages/react-reconciler/src/ReactStrictModeWarnings.js
View file
Open in desktop
Expand Up @@ -141,12 +141,18 @@ if (__DEV__) {

lowPriorityWarning(
false,
'componentWillMount is deprecated and will be removed in the next major version. ' +
'Use componentDidMount instead. As a temporary workaround, ' +
'you can rename to UNSAFE_componentWillMount.' +
'\n\nPlease update the following components: %s' +
'\n\nLearn more about this warning here:' +
'\nhttps://fb.me/react-async-component-lifecycle-hooks',
'componentWillMount has been renamed to UNSAFE_componentWillMount, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
'- If you initialize state in componentWillMount, move this logic into the constructor.\n' +
'- If you fetch data or perform other side effects in componentWillMount, ' +
'move this logic into componentDidMount.\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should give a hint as to why folks might not want to just rename and ignore? Maybe we could replace

"To rename all deprecated lifecycles to their new names, ..."

...with

"To prevent this warning from being logged to the console, ..."

...so it sounds a little more like you're just choosing to ignore the warning without fixing the problem?

(here and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "To ignore this warning from being logged to the console..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather, "to disable this warning..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's so wordy :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we're saying the warning will still log in strict mode, and that we've moved this to the bottom of the list, is sufficient. I'll fight you on this briannson.

'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: %s\n' +
'\nLearn more about this warning here:\n' +
'https://fb.me/react-async-component-lifecycle-hooks',
sortedNames,
);

Expand All @@ -164,11 +170,21 @@ if (__DEV__) {

lowPriorityWarning(
false,
'componentWillReceiveProps is deprecated and will be removed in the next major version. ' +
'Use static getDerivedStateFromProps instead.' +
'\n\nPlease update the following components: %s' +
'\n\nLearn more about this warning here:' +
'\nhttps://fb.me/react-async-component-lifecycle-hooks',
'componentWillReceiveProps has been renamed to UNSAFE_componentWillReceiveProps, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
"- If you're updating state whenever props change, " +
'move this logic into static getDerivedStateFromProps.\n' +
'- If you fetch data or perform other side effects in componentWillReceiveProps, ' +
'move this logic into componentDidUpdate.\n' +
'- Refactor your code to use memoization techniques instead of derived state. ' +
'Learn more at: https://fb.me/react-derived-state\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: %s\n' +
'\nLearn more about this warning here:\n' +
'https://fb.me/react-async-component-lifecycle-hooks',
sortedNames,
);

Expand All @@ -186,12 +202,19 @@ if (__DEV__) {

lowPriorityWarning(
false,
'componentWillUpdate is deprecated and will be removed in the next major version. ' +
'Use componentDidUpdate instead. As a temporary workaround, ' +
'you can rename to UNSAFE_componentWillUpdate.' +
'\n\nPlease update the following components: %s' +
'\n\nLearn more about this warning here:' +
'\nhttps://fb.me/react-async-component-lifecycle-hooks',
'componentWillUpdate has been renamed to UNSAFE_componentWillUpdate, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
'- If you fetch data or perform other side effects in componentWillUpdate, ' +
'move this logic into componentDidUpdate.\n' +
"- If you're reading DOM properties before an update, " +
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legit use cases for getSnapshotBeforeUpdate seem so uncommon, I wonder if it's worth mentioning here at all? I don't have a strong preference one way or another, but a slight preference to maybe not mention it here for fear of encouraging bad patterns for those maybe lacking context of when the method should be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll leave it here. We seem fairly comprehensive for the others, it would be frustrating if the dev had to go to a doc to discover a new api for just this one case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm concerned that if people just move stuff to that method, without the added context of why it exists, they might be tempted to do mutations/side effects in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They would probably try componentDidUpdate before that though? I dunno tbh. I could remove this, but seems like we should use same reasoning to remove getDerivedStateFromProps if so.

'move this logic into getSnapshotBeforeUpdate.\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: %s\n' +
'\nLearn more about this warning here:\n' +
'https://fb.me/react-async-component-lifecycle-hooks',
sortedNames,
);

Expand Down
8 changes: 4 additions & 4 deletions packages/react/src/__tests__/ReactStrictMode-test.internal.js
View file
Open in desktop
Expand Up @@ -429,9 +429,9 @@ describe('ReactStrictMode', () => {
);
}).toLowPriorityWarnDev(
[
'componentWillMount is deprecated',
'componentWillReceiveProps is deprecated',
'componentWillUpdate is deprecated',
'componentWillMount has been renamed',
'componentWillReceiveProps has been renamed',
'componentWillUpdate has been renamed',
],
{withoutStack: true},
);
Expand Down Expand Up @@ -517,7 +517,7 @@ describe('ReactStrictMode', () => {
'\n\ncomponentWillMount: Please update the following components ' +
'to use componentDidMount instead: Baz',
]);
}).toLowPriorityWarnDev(['componentWillMount is deprecated'], {
}).toLowPriorityWarnDev(['componentWillMount has been renamed'], {
withoutStack: true,
});

Expand Down
30 changes: 23 additions & 7 deletions packages/react/src/__tests__/createReactClassIntegration-test.internal.js
View file
Open in desktop
Expand Up @@ -51,10 +51,16 @@ describe('create-react-class-integration', () => {
});

expect(() => ReactNative.render(<View />, 1)).toLowPriorityWarnDev(
'componentWillMount is deprecated and will be removed in the next major version. ' +
'Use componentDidMount instead. As a temporary workaround, ' +
'you can rename to UNSAFE_componentWillMount.' +
'\n\nPlease update the following components: MyNativeComponent',
'componentWillMount has been renamed to UNSAFE_componentWillMount, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
'- If you initialize state in componentWillMount, move this logic into the constructor.\n' +
'- If you fetch data or perform other side effects in componentWillMount, ' +
'move this logic into componentDidMount.\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: MyNativeComponent\n',
{withoutStack: true},
);
});
Expand All @@ -68,9 +74,19 @@ describe('create-react-class-integration', () => {
});

expect(() => ReactNative.render(<View />, 1)).toLowPriorityWarnDev(
'componentWillReceiveProps is deprecated and will be removed in the next major version. ' +
'Use static getDerivedStateFromProps instead.' +
'\n\nPlease update the following components: MyNativeComponent',
'componentWillReceiveProps has been renamed to UNSAFE_componentWillReceiveProps, ' +
"and the old name won't work in the next major version of React.\n\n" +
'We suggest doing one of the following:\n' +
"- If you're updating state whenever props change, " +
'move this logic into static getDerivedStateFromProps.\n' +
'- If you fetch data or perform other side effects in componentWillReceiveProps, ' +
'move this logic into componentDidUpdate.\n' +
'- Refactor your code to use memoization techniques instead of derived state. ' +
'Learn more at: https://fb.me/react-derived-state\n' +
'- To rename all deprecated lifecycles to their new names, you can run ' +
'`npx react-codemod rename-unsafe-lifecycles <path/to/code>` in your project folder. ' +
'(Note that the warning will still be logged in strict mode.)\n' +
'\nPlease update the following components: MyNativeComponent\n',
{withoutStack: true},
);
});
Expand Down