FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix warning at #2680 by icecream17 · Pull Request #2681 · atom/github · GitHub

This repository was archived by the owner on Dec 15, 2022. It is now read-only.
/ github Public archive

fix warning at #2680 - #2681

Open
icecream17 wants to merge 3 commits into
atom:masterfrom
icecream17:fix-warning
ConversationCommits (3)ChecksFiles changed
Open

fix warning at #2680#2681
icecream17 wants to merge 3 commits into
atom:masterfrom
icecream17:fix-warning

Conversation

icecream17 commented May 14, 2021 •
edited
Loading

Copy link
Copy Markdown

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Description of the Change

componentWillRecieveProps is now deprecated, so this pr replaces it.

https://reactjs.org/docs/react-component.html#unsafe_componentwillreceiveprops says:
"If you need to perform a side effect (for example, data fetching or an animation) in response to a change in props, use componentDidUpdate lifecycle instead."

componentDidUpdate is called after updating, rather than componentWillRecieveProps.
If this (or some other subtle change) happens to break atom, then I'll just use UNSAFE_componentWillReceiveProps instead.

Warning text:

Warning: componentWillReceiveProps has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles 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.

In this case, it's used for side effects (1st bullet point)

And this implies there will still be a warning when using UNSAFE_componentWillReceiveProps in strict-mode, so trying componentDidUpdate first.

Screenshot or Gif

N/A

Applicable Issues

Fixes Part of Fixes #2680

icecream17 commented May 14, 2021

Copy link
Copy Markdown
Author

The tests failed, but for external reasons

Also there's this:

smashwilson approved these changes May 14, 2021
View reviewed changes

smashwilson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should work 👍🏻

It looks like we have another use of componentWillReceiveProps in the codebase, though. Can we leave the linked issue open until that's converted, as well?

smashwilson commented May 14, 2021

Copy link
Copy Markdown
Contributor

The tests failed, but for external reasons

Yeah, our CI seems really unhealthy 😒 . It was always a bit rickety even when this was my major focus, but it looks like entropy has made it even worse since the last time I've done work here.

This was referenced May 14, 2021
Merged
Open
copypaste previous commit
ed14d2b 
`_prevProps` is unused, that's why it has an underscore. Maybe atom does something differently?

codecov Bot commented Sep 2, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #2681 (8b1cfaf) into master (2053290) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2681   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files         237      237           
  Lines       13213    13213           
  Branches     1900     1900           
=======================================
  Hits        12349    12349           
  Misses        864      864
Impacted Files Coverage Δ
lib/atom/commands.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2053290...8b1cfaf. Read the comment docs.

icecream17 commented Sep 2, 2021

Copy link
Copy Markdown
Author

That last commit was for the tests to run

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
smashwilson smashwilson approved these changes
unrealapex unrealapex approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Warning: componentWillReceiveProps has been renamed, and is not recommended for use.

4 participants

Back | FazBrowse Home | New Git URL