-
Notifications
You must be signed in to change notification settings - Fork 193
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
Fix componentWillReceiveProps warnings #176
Conversation
So we can test componentWillReceiveProps changes
@@ -795,7 +794,7 @@ export class UnControlled extends React.Component<IUnControlledCodeMirror, any> | |||
let update = true; | |||
|
|||
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
componentWillReceiveProps
tocomponentDidUpdate
(and swap thenextProps
andprevProps
accordingly). Since the render function just returns a static container element (that is, nothing that happens incomponentWillReceiveProps
affects the render), it's safe to move everything before the render incomponentWillReceiveProps
to after the render incomponentDidUpdate
.Tests pass, and I verified with the
npm run start
test app that the editor continues to respect prop changes.Additional changes
npm audit fix
to fix anode-sass
vulnerability that NPM was complaining about.