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

Console warnings for soon-to-be-deprecated React lifecycle methods #492

Closed
pebcakerror opened this issue Oct 18, 2019 · 13 comments
Closed
Milestone

Comments

@pebcakerror
Copy link

React will be deprecating some of its lifecycle methods in the next major version. https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

React 16.9 started displaying console warnings about the soon-to-be-deprecated methods. https://reactjs.org/blog/2019/08/08/react-v16.9.0.html

Warning: componentWillMount has been renamed, and is not recommended for use. See https://fb.me/react-unsafe-component-lifecycles for details.

* Move code with side effects to componentDidMount, and set initial state in the constructor.
* Rename componentWillMount to UNSAFE_componentWillMount 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: FixedDataTable, FixedDataTableBufferedRows, FixedDataTableCellGroupImpl, FixedDataTableContainer, FixedDataTableRow, HorizontalScrollbar, Scrollbar
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.

Please update the following components: ColumnResizerLine, FixedDataTable, FixedDataTableCell, FixedDataTableContainer, Scrollbar
@pebcakerror
Copy link
Author

You can use https://github.com/reactjs/react-lifecycles-compat to maintain backwards compatibility.

@pradeepnschrodinger
Copy link
Collaborator

Hey. sorry for the late response.

I think instead of using UNSAFE_* methods, we should just avoid componentWillMount and maybe make use of getDerivedStateFromProps for componentWillReceiveProps.
Not sure how feasible this is, especially for components like FixedDataTableContainer, which is tied to the Redux store, but I think this is definitely worth looking into.

We're happy to look into a PR.

@wcjordan wcjordan added this to the v1.1 milestone Mar 3, 2020
@wcjordan
Copy link
Member

wcjordan commented Apr 14, 2020

This is mostly fixed by #509 and v1.1. There's still one remaining use of deprecated lifecycle methods which couldn't be removed w/o introducing breaking behavior. We're investigating further and will leave this open while we address that issue.

@wcjordan wcjordan modified the milestones: v1.1, v1.2 May 4, 2020
@HillLiu
Copy link

HillLiu commented Jul 29, 2020

There is still one componentWillReceiveProps.

componentWillReceiveProps(nextProps) {

@binarykitchen
Copy link

Sorry, it's too much noise in the console.

Can we fix this temporarily by amending UNSAFE_ for now? And then can take your time to complete the proper fixes as mentioned above.

If you like that idea, can do a PR ...

@mrmcduff
Copy link

@wcjordan -- what is the breaking behavior that comes from replacing the method called in FixedDataTableContainer.js? I can pass tests and render the sample server okay when just mirroring the change in #509

#599

@wcjordan
Copy link
Member

@pradeepnschrodinger is better to discuss the issue. I believe there are issues related to the synchronization of the component and the redux store.

@mrmcduff
Copy link

Thanks for the response! I'd love to find out more - I updated the PR description to match guidelines in case the fix is as simple as that; totally understand if there's something under the hood with the redux store that I'm missing.

@wcjordan
Copy link
Member

Looking at this comment some of this may also be driven by a desire to not drop support for React < 16.3 until we can do a minor release.

@mrmcduff
Copy link

I actually didn't use the getDerivedStateFromProps method which appears to be the root cause of the deprecation -- #509 was merged, which made identical changes to the one (only one change) in my PR, essentially swapping out componentWillReceiveProps with componentDidUpdate, switching references in that method with nextProps -> this.props and this.props -> prevProps, along with adding that guard at the start of the method to avoid loops due to internal updates.

@mrmcduff
Copy link

Apologies; messed up the PR procedure and didn't realize I was default making a PR to the main project with the one that needed to merge to my forked master branch first. Updated with #600

@pradeepnschrodinger
Copy link
Collaborator

The breaking behavior when we rely on componentDidUpdate in place of FDTContainer's componentWillReceiveProps is discussed on #523, which is linked to #509.

Basically, when there's new props, FDT's redux store is only updated with the props AFTER the table is rendered, because store update is tied to componentDidUpdate.
This leads to two problems --

  • we have two renders for each prop change, which can cause performance issues when we have controlled scrolling (eg: AutoScroll Example)
  • in the first render, components will be using the redux store state that might not match the new props.

The second point is a breaking change -- during the first render, the cell renderers will be called with mismatched/stale props because the redux store has not been updated with the new props yet.

@pradeepnschrodinger
Copy link
Collaborator

Fixed on v1.2.6 through #509 and #612.

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

No branches or pull requests

6 participants