update deprecated lifecycle methods

[tarekrached]

Super naive approach - I basically switched componentWillMount to componentDidMount and componentWillUpdate to componentDidUpdate. componentDidUpdate receives prevProps
instead of nextProps, so I also changed this.materializeInput(nextProps.data) to this.materializeInput(this.props.data).

npm run test:jest passes, but npm run test fails at the prettier stage.

FWIW there's still a componentWillMount and a componentWillUpdate in examples/App.jsx but I didn't want to muck with that...

[tarekrached]

seeing some issues, will investigate and reopen

[tarekrached]

Ok, I figured out what was going on and dealt with it. data, materializedInput, and attrValues were all class properties of the PivotTableUI component, as opposed to being in the React component state, so running this.materializeInput() in componentDidMount had no effect because the updates to materializedInput and attrValues were not visible to React.

I moved data, materializedInput, and attrValues off of the class and into the component state, and it appears to work. Tests still pass, etc, etc. This is a superficial pass that had to go a bit deeper than I anticipated, so it probably deserves scrutiny, and ideally testing in some varied environments.

I also added all the build artifacts to the .gitignore bc I nearly checked them in by accident. Might make sense to make a top-level build/ directory to avoid cluttering the root...

[nicolaskruchten]

Thanks for this! I'll try to review it early next week as I'm travelling at the moment :)

[tarekrached]

hey @nicolaskruchten, any update on this? we'd love to see this merged and released so we don't need to use an internal fork...

[nicolaskruchten]

just for reference, which versions of React is this meant to be compatible with?

[tarekrached]

Here's the official notice from the React team on the migration path away from the deprecated lifecycle methods (componentWillMount, componentWillReceiveProps, and componentWillUpdate).

The warnings started showing up in 16.9.0, and the lifecycle methods will be removed in 17.0. Without this PR, react-pivottable will warn in dev mode in current (^16.9) react, and will not work in 17.x whenever that comes out.

[nicolaskruchten]

OK, cool, but then how far back are these changes meant to stay compatible? Will this still work with React 15? 14? etc :)

[tarekrached]

As far as I can tell from a cursory look at the React CHANGELOG, this should work with all older versions of React, as the only thing I'm doing here is moving to componentDidMount and componentDidUpdate, which have been around since at least v0.3.0. That said, I have not tested with any older version of react than whatever was latest at the time of this PR.

[nicolaskruchten]

OK, this looks great, thanks so much for the PR :) It kind of seems like this whole materializedInput thing is a bit of an anti-pattern actually, per https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#when-to-use-derived-state ... I mean it was before this PR and now that we're using state it's clearer that this is an anti-pattern.

[nicolaskruchten]

Published in https://github.com/plotly/react-pivottable/releases/tag/v0.10.0 :)

[tarekrached]

🎉