Skip to content

Migrate from legacy to new React lifecycle#127

Open
rbardini wants to merge 1 commit into
jamiebuilds:masterfrom
rbardini:lifecycle-migration
Open

Migrate from legacy to new React lifecycle#127
rbardini wants to merge 1 commit into
jamiebuilds:masterfrom
rbardini:lifecycle-migration

Conversation

@rbardini

Copy link
Copy Markdown

React will soon deprecate componentWillMount, componentWillReceiveProps and componentWillUpdate methods due to their potential misuse, especially once async rendering is launched. In fact, enabling strict mode already warns about these unsafe lifecycle methods usage.

This PR moves module loading logic from componentWillMount to componentDidMount, which is the recommended upgrade path in this case.

@tajo

tajo commented Jun 29, 2018

Copy link
Copy Markdown
Collaborator

Did you test this with regards of SSR (cWM is called but cDM not)?

@rbardini

rbardini commented Jul 8, 2018

Copy link
Copy Markdown
Author

I assumed tests were already covering this. Are there any use cases missing in this regard?

@mr-pinzhang

Copy link
Copy Markdown

isn't this should be migrate to constructor as new lifecycle suggested?

@roccoluke

Copy link
Copy Markdown

Any timeframe as to when to expect this to me merged?

@amakhrov

amakhrov commented Aug 6, 2018

Copy link
Copy Markdown

@tajo @jamiebuilds any feedback on that?

@tajo tajo closed this Aug 9, 2018
@tajo tajo reopened this Aug 9, 2018
@titouancreach

Copy link
Copy Markdown

@bjminhuang I don't think so. If you do that, you'll potentially add a side effect (this.setState) in the constructor that is not recommended. The React way for doing this is componentDidMount

@the0neWhoKnocks

Copy link
Copy Markdown

Would really like to see this get merged. Currently trying to integrate this with a project on React 16.4 and the use of componentWillMount is causing the component not to render properly for SSR. I went in to the react-loadable module and swapped componentWillMount for componentDidMount and no more warnings or SSR issues.

@the0neWhoKnocks

Copy link
Copy Markdown

Actually, looks like this may have partially been addressed by this merged PR #123, although componentWillMount still remains along with the added componentDidMount.

@the0neWhoKnocks

Copy link
Copy Markdown

Any traction on this? The PR appears ready to merge.

@dbenchi

dbenchi commented Sep 10, 2018

Copy link
Copy Markdown

Any news about this PR?

@mshwery

mshwery commented Sep 20, 2018

Copy link
Copy Markdown

LGTM! Anything I can do to help move this along?

@jaybe78 jaybe78 mentioned this pull request Oct 22, 2018
@jaybe78

jaybe78 commented Oct 23, 2018

Copy link
Copy Markdown

@abenchi @mshwery
This includes your PR + migration to babel .7 and webpack 5
https://www.npmjs.com/package/jaybe-react-loadable
Cheers

@jedwards1211

jedwards1211 commented Nov 15, 2018

Copy link
Copy Markdown

@jamiebuilds can we please get this merged? Do you need a new maintainer?

@courtyenn

courtyenn commented Dec 17, 2018

Copy link
Copy Markdown

Would love to see this make some progress.

@jaybe78

jaybe78 commented Dec 18, 2018

Copy link
Copy Markdown

@abenchi yes you can lazy load with latest version of react, though suspense is not supported in SSR mode... So for people like me who server side render their app, there's no option beside react universal or react loadable

Repository owner deleted a comment from SimenB Feb 28, 2020
Repository owner deleted a comment from dbenchi Feb 28, 2020
@owen2345

owen2345 commented Aug 5, 2020

Copy link
Copy Markdown

Hey @jamiebuilds @tajo
any news to fix the concerning issue of this PR?

@slorber

slorber commented Feb 12, 2021

Copy link
Copy Markdown

For anyone interested, here's my solution to get rid of the warning (React 16 + 17) and still get this working (client+SSR).

#213 (comment)

Repository owner deleted a comment from ygs-code Sep 6, 2022
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

Successfully merging this pull request may close these issues.