-
Notifications
You must be signed in to change notification settings - Fork 194
Fix componentWillReceiveProps warnings #176
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
Conversation
So we can test componentWillReceiveProps changes
|
|
||
| if (SERVER_RENDERED) update = false; | ||
| if (this.detached) update = false; | ||
| if (this.detached && nextProps.detach) update = false; |
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.
This is the only change outside of componentWillReceiveProps / componentDidUpdate. This is necessary (tests fail otherwise) because shouldComponentUpdate runs after componentWillReceiveProps but before componentDidUpdate. Without this change, once we were in a detached state, we would ignore any subsequent attempts via prop changes to re-attach.
|
Would be great if this was merged so that we can stop seeing the warnings. Great work! |
|
I don't understand why checks failed |
|
The checks all passed except for a minor drop in test coverage. I think it dropped because the PR adds one extra condition to safeguard here (https://github.com/scniro/react-codemirror2/pull/176/files#discussion_r373346723) but given that test coverage is already pretty decent, I think it's fine? |
|
how about simply modify the name to UNSAFE_xx ? |
|
@matteokjh Why? Then this module will just break when React 17 lands. |
|
@scniro Can you review this PR? Is the coveralls warning actually blocking or can you merge despite it? |
|
@scniro can you please review and merge this? |
|
@Chr1stian merged and published with the 6.0.1 release. Let me know if this is better? I likely will not be actively maintaining this project much moving forward, let me know if you'd like to be a co-maintainer? Would gladly pass the torch if you'd like to help out 😄 |
|
I wouldn't mind helping out a bit either. This is a dependency for some stuff at work, so I have a small interest in keeping it up to date. |
|
@fongandrew most excellent to hear! I've invited you to collaborate. Unsure how this works fully, ensuring you'd have permissions for everything. Shoot me an email if you need anything else to get going. I'll be curious to see if you can publish to npm. Thanks again! |
|
Thank you very much @scniro ! Seems to be working just fine and without the warnings now 👍 |
This should fix the warnings from #134 and #152.
The main change in this PR is in 6d57fe0. We just change
componentWillReceivePropstocomponentDidUpdate(and swap thenextPropsandprevPropsaccordingly). Since the render function just returns a static container element (that is, nothing that happens incomponentWillReceivePropsaffects the render), it's safe to move everything before the render incomponentWillReceivePropsto after the render incomponentDidUpdate.Tests pass, and I verified with the
npm run starttest app that the editor continues to respect prop changes.Additional changes
npm audit fixto fix anode-sassvulnerability that NPM was complaining about.