schrodinger/fixed-data-table-2

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

Closed this issue ยท 13 comments

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

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

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.

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.

There is still one componentWillReceiveProps.

componentWillReceiveProps(nextProps) {

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 ...

@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

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

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.

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.

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.

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

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.

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