Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Remove usages of unsafe lifecycle methods #406

Closed
Hypnosphi opened this issue Mar 29, 2018 · 10 comments
Closed

Remove usages of unsafe lifecycle methods #406

Hypnosphi opened this issue Mar 29, 2018 · 10 comments

Comments

@Hypnosphi
Copy link

React team recently published a blogpost: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html

TL;DR: cWM, cWRP, and cWU methods are considered unsafe for future async rendering and will be deprecated soon. The recommended way for open source maintainers to deal with it is to use react-lifecycles-compat package and migrate to new static getDerivedStateToProps method

@kentcdodds
Copy link
Contributor

Thanks! I don't have time to work on this now myself but I'd love a PR for it :)

@stevenandersonz
Copy link

There is somebody working on this already? I would love to help here!

@Hypnosphi
Copy link
Author

Hypnosphi commented Apr 3, 2018

I tried to start but hit #407. Feel free to pick it anyway

@stevenandersonz
Copy link

Thank you, just started making the changes to theme-provider.js, I remove the component life-cycle method componentWillReceiveProps and instead I added the new static getDerivedPropsFromState() to make this work I need to add a new property to the state and implement componentDidUpdate() as I show below:

import polyfill from "react-lifecycles-compat";

class ThemeProvider extends React.Component {
  state = {
    theme: null
  };
  static getDerivatedPropsFromState(nextProps, prevState) {
    if (prevState.theme !== nextProps.theme) {
      return { theme: nextProps.theme };
    }
    return null;
  }
  componentDidUpdate(prevProps, prevState) {
    if (this.state.theme !== prevState.theme) {
      this.publishTheme(this.state.theme);
    }
  }
  UNSAFE_componentWillMount() {
    // set broadcast state by merging outer theme with own
    if (this.context[CHANNEL]) {
      this.setOuterTheme(this.context[CHANNEL].getState());
    }
    this.broadcast = brcast(this.getTheme(this.props.theme));
  }
  //all the functions here still the same
}
polyfill(ThemeProvider)
export default ThemeProvider

I ran npm test and all the test pass, is this approach okay?

@Hypnosphi
Copy link
Author

Hypnosphi commented Apr 4, 2018

can the cWM be replaced with cDM or constructor?

@kentcdodds
Copy link
Contributor

Maybe. I'll be honest and say that I only barely understand what's going on with this code. I'd actually prefer to rewrite this whole Theming system using the new context API (with create-react-context as a polyfill).

@Hypnosphi
Copy link
Author

Feel free to borrow ideas from #styled-components/styled-components#1664

Particularly, here's the solution for ThemeProvider lifecycles: https://github.com/styled-components/styled-components/pull/1664/files#diff-9f2631c972fc5bab75b5dee186444983

@Hypnosphi
Copy link
Author

Hypnosphi commented Apr 8, 2018

rewrite this whole Theming system using the new context API

That's a good idea, but that would mean deprecating React <16 actually it wouldn't, see jamiebuilds/create-react-context#15

@sscaff1
Copy link

sscaff1 commented May 2, 2018

I would like to work on this using @kentcdodds suggestion above regarding the theme-provider.

@kentcdodds
Copy link
Contributor

This project is now in an unmaintained status. Please see the README for more information.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants