-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
Remove old UNSAFE_ lifecycle methods #5020
Conversation
Are head update reflected in time before rendering when doing this though? I remember @giuseppeg saying it works fine for styled-jsx, so maybe he can weight in 🕵️ Thank you very much for this contribution, helps a lot 😌 |
@timneutkens I believe so, since constructor is called before |
If side effects are synchronous it should be good :) |
@@ -77,9 +76,6 @@ export default function withSideEffect (reduceComponentsToState, handleStateChan | |||
} | |||
} | |||
|
|||
// Make UNSAFE_ compatible with version of React under 16.3 | |||
polyfill(SideEffect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timneutkens @giuseppeg I see that a couple tests still use UNSAFE_
. . . do you want me to keep react-lifecycles-compat
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests don't need the compat because we run tests on 16.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So removing it is perfect 👍
I just realized we don't have integration tests for routing with next/head, could you add a few? Otherwise, I'll add them 👍 |
@timneutkens I'm not sure if I have time right now so you will probably get it done a lot sooner than me, but I'll try and get to it 👍 |
Actually @timneutkens I'm not exactly sure what you mean by:
It would be helpful to have some clarification on what exactly to test for, and if I should create a new folder or add it to an existing one. |
You can extend |
Thank you very much for adding the test, I've updated it a bit, this is ready to be merged after tests pass 😌 |
Thanks Tim 👍 |
Any chance this broke |
@SBoudrias could you create a minimal reproduction so that we can add a test for it? |
I am not sure if this is a valid fix yet, but I was going to let CI run the tests for me. I'll close and look into it if the build fails.Let me know if this will cause issues, but I don't think it should. The React docs recommends moving
componentWillMount
logic into the constructor