Skip to content

Conversation

@lukyth
Copy link

@lukyth lukyth commented Aug 11, 2019

Fixes #3361 (comment)

Checklist

  • [ ] Includes tests (Not sure what to test. Maybe a lint rule to prevent adding deprecated lifecycle in the future? But I can't find such rule in tslint-react.)
  • [ ] Update documentation (No documentation to update.)

Changes proposed in this pull request:

As pointed by #3361 (comment), with React 16.9 released, there'll be a warning in the console for every deprecated lifecycle usage.

 Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://fb.me/react-async-component-lifecycle-hooks for details.

     * Move data fetching code or side effects to componentDidUpdate.
     * If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state
     * Rename componentWillReceiveProps to UNSAFE_componentWillReceiveProps to suppress this warning in non-strict mode. In React 17.x, only the UNSAFE_ name will work. To rename all deprecated lifecycles to their new names, you can run `npx react-codemod rename-unsafe-lifecycles` in your project source folder.

     Please update the following components: Blueprint3.Overlay, Blueprint3.Toaster

This PR fixes those warnings by adding UNSAFE_ prefix to every deprecated lifecycle.

I think this is the best approach because it's the easiest way to solve the issue, and we can be certain it's not gonna break anything (which might be the case if we migrate to another lifecycle method). We can migrate all these UNSAFE_ lifecycles gradually later on.

But this UNSAFE_* lifecycle is not in React 15, so we'll have to drop support for it. Is this acceptable?

I also renamed some jobs in the CI to reflect the change of dropping React 15, which makes the CI jobs name mismatched between GitHub and CircleCI.

GitHub CircleCI
Screen Shot 2019-08-14 at 16 43 47 Screen Shot 2019-08-14 at 16 43 28
test-react-16 test
test-react-15 -
test-iso-react-16 test-iso
test-iso-react-15 -

Reviewers should focus on:

  • The correctness of the changes.
  • React 15 support is dropped.
  • CI got changed a bit (remove -react-15 jobs from the workflow).
  • test-react15 package is removed.

Screenshot

None

@ro-savage
Copy link

I think this is the wrong approach.

componentWillReceiveProps should be removed completely and replaced by componentDidUpdate and getDerivedStateFromProps

It's obviously a bigger update, but I believe that will support React-15 and React-17

@tobilen
Copy link
Contributor

tobilen commented Aug 14, 2019

afaik, react 15 does not support getDerivedStateFromProps. For that matter, nothing below 16.3 is able to deal with the UNSAFE_ methods.

You should change the peerDepdencencies in the package.json to require react version >=16.3 instead of the current ^15.3.0 || 16.

@lukyth lukyth force-pushed the deprecate-lifecycle branch from 17caf85 to 488535f Compare August 14, 2019 07:26
@lukyth lukyth force-pushed the deprecate-lifecycle branch from 488535f to 984d821 Compare August 14, 2019 07:30
@tobilen
Copy link
Contributor

tobilen commented Aug 14, 2019

fyi, i am currently working on a proper replacement of life cycle methods in my fork. This will then be able to use the react teams polyfill to remain compatible to react 15 or below.

this is still a couple of days from being a proper PR, and even then i might need help with parts of the codebase. for that reason i am in favor of pushing the fix proposed here as a prerelease so people can at least get rid of noisy deprecation warning spam in their test output

edit: #3702

@pierreneter
Copy link

We should not use UNSAFE_ prefix. Please replace componentWillReceiveProps by componentDidUpdate and getDerivedStateFromProps.

Copy link

@pierreneter pierreneter left a comment

Choose a reason for hiding this comment

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

Don't use UNSAFE_ prefix.

@adidahiya adidahiya self-assigned this Aug 19, 2019
@atocqueville
Copy link

Hello, any update on this ?

@adidahiya
Copy link
Contributor

Closing this in favor of #3702, which I'm reviewing now

@adidahiya adidahiya closed this Sep 18, 2019
@adidahiya adidahiya removed their assignment Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support React Strict Mode

6 participants