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

Connected components cannot use new 16.3 lifecycles #915

Closed
byronanderson opened this issue Mar 30, 2018 · 12 comments
Closed

Connected components cannot use new 16.3 lifecycles #915

byronanderson opened this issue Mar 30, 2018 · 12 comments

Comments

@byronanderson
Copy link

I know that there is still much consideration going into #890 so I don't mean to rush, but I thought you might be interested to know:

It looks like React 16.3 exposes the new lifecycles in a way that disallow for using the old componentWill* at the same time as new lifecycles.

Something that looks roughly like this:

class Form extends PureComponent {
  constructor(props) {
    super(props);
    this.state = {initialized: false};
  }
  static getDerivedStateFromProps(nextProps, prevState) {
    if (nextProps.data && !prevState.initialized) {
      return {initialized: true};
    }
    return null;
  }
  render() {
    return this.state.initialized ? "initialized" : "uninitialized"
  }
}
export default connect()(Form);

will get this warning:

screen shot 2018-03-30 at 9 24 26 am

which I didn't understand until I saw that the implementation details of connect use inheritance to add those lifecycles.

My workaround in this first case I found in my code is pretty easy to fix (I was just using the connect to get a dispatch method) but I bet people are going to start running into this as they upgrade to 16.3.

Sending my best to your efforts in #890 !

@markerikson
Copy link
Contributor

I'm not sure I understand that example. The Form component contains neither cWRP nor cWU.

Can you put together a minimal repro of this issue in a CodeSandbox or something?

@byronanderson
Copy link
Author

Sure thing...

@markerikson
Copy link
Contributor

markerikson commented Mar 30, 2018

Hmm. Perhaps it's due to our use of hoist-non-react-statics ? Except that v2.5.0 added support for gDSFP, and @timdorr switched us over to use that a month ago. It looks like react-redux@5.0.7 ought to resolve this.

@byronanderson
Copy link
Author

I was having a heck of a time reproducing with most recent versions of dependencies on codesandbox, so that would make sense...

I'll upgrade and check...

@byronanderson
Copy link
Author

Yep, you are right, most recent patch version fixes this.

Thanks for the quick feedback, and sorry for wasting your time!

@swac
Copy link

swac commented Apr 9, 2018

Is this actually addressed? It seems like connectAdvanced is still injecting a componentWillUpdate function when in development mode: https://github.com/reactjs/react-redux/blob/c1b9d36e078836374fe833a6eb2a8f0de0ad1e30/src/components/connectAdvanced.js#L268-L291

If I understand React's warning correctly, this means the componentWillUpdate function in connectAdvanced won't be called for a component that's been migrated to use a new lifecycle method.

@markerikson
Copy link
Contributor

The problem here is that getDerivedStateFromProps was being unnecessarily hoisted into the generated wrapper component. v5.0.7 fixes this, because it uses a newer version of hoist-react-statics that skips gDSFP.

We are still using a componentWillUpdate method in 5.0.7, but React isn't warning about that yet, and we're working it in #919 anyway.

@vctormb
Copy link

vctormb commented Apr 20, 2018

@markerikson is it correct to dipatch() something inside gDSFP or inside cDU?

@gaearon
Copy link
Contributor

gaearon commented Apr 20, 2018

No, gDSFP must be pure and contain no dispatch calls.

Dispatching in cDU is okay-ish. Although it's still not that great from performance perspective to dispatch in response to re-rendering. But you were already doing it.

@vctormb
Copy link

vctormb commented Apr 20, 2018

@gaearon Actually, I already have it inside cWRP (if props change, I compare it and dispatch something to reducer).. but now that I've updated the react version I'm trying to migrate it. Humm.. is there a correct way to dispatch something when props change?

@timdorr
Copy link
Member

timdorr commented Apr 20, 2018

@vctormb cDU is the recommended way: https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#side-effects-on-props-change

But I'd probably ask what your cWRP is responding to. Could that be responsible for dispatching the action? Could you push the state transformation back to the store itself? There is probably a larger refactor that will actually put you in a better position not just for React async stuff, but as your app changes over time.

@vctormb
Copy link

vctormb commented Apr 20, 2018

@timdorr Yes! I already read this blog! Btw, it is really great, thanks! Well, this dispatch I'm using to clear some values inside reducer because they are related to redux-form. So I need to use some action creators to change some values there.

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