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

Fix componentWillReceiveProps warnings #176

Merged
merged 4 commits into from
Mar 15, 2020
Merged

Conversation

fongandrew
Copy link
Collaborator

This should fix the warnings from #134 and #152.

The main change in this PR is in 6d57fe0. We just change componentWillReceiveProps to componentDidUpdate (and swap the nextProps and prevProps accordingly). Since the render function just returns a static container element (that is, nothing that happens in componentWillReceiveProps affects the render), it's safe to move everything before the render in componentWillReceiveProps to after the render in componentDidUpdate.

Tests pass, and I verified with the npm run start test app that the editor continues to respect prop changes.

Additional changes

  • Updated React dependencies so we can repro the error message while testing
  • Ran npm audit fix to fix a node-sass vulnerability that NPM was complaining about.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 81.022% when pulling 8b2d5d0 on fongandrew:master into a633e7d on scniro:master.

@@ -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;
Copy link
Collaborator Author

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.

@Chr1stian
Copy link

Would be great if this was merged so that we can stop seeing the warnings. Great work!

@matteokjh
Copy link

I don't understand why checks failed

@fongandrew
Copy link
Collaborator Author

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?

@matteokjh
Copy link

how about simply modify the name to UNSAFE_xx ?

@fongandrew
Copy link
Collaborator Author

@matteokjh Why? Then this module will just break when React 17 lands.

@fongandrew
Copy link
Collaborator Author

@scniro Can you review this PR? Is the coveralls warning actually blocking or can you merge despite it?

@Chr1stian
Copy link

@scniro can you please review and merge this?

@scniro scniro merged commit e299390 into scniro:master Mar 15, 2020
@scniro
Copy link
Owner

scniro commented Mar 15, 2020

@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 😄

@fongandrew
Copy link
Collaborator Author

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.

@scniro
Copy link
Owner

scniro commented Mar 15, 2020

@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!

@Chr1stian
Copy link

Thank you very much @scniro ! Seems to be working just fine and without the warnings now 👍
Currently only using it for a shortlived small project, but if I ever need it as a dependency in a production-app I would be up for the task. Great to see that @fongandrew already has done some work 💯

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.

5 participants