search

scniro / react-codemirror2

Codemirror integrated components for React
MIT License
1.66k stars 193 forks source link

Fix componentWillReceiveProps warnings #176

Closed fongandrew closed 4 years ago

fongandrew commented 4 years ago

This should fix the warnings from https://github.com/scniro/react-codemirror2/issues/134 and https://github.com/scniro/react-codemirror2/issues/152.

The main change in this PR is in https://github.com/scniro/react-codemirror2/commit/6d57fe01720b86f99f182e402ea42d15739c9a99. 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

coveralls commented 4 years ago

Coverage Status

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

Chr1stian commented 4 years ago

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

matteokjh commented 4 years ago

I don't understand why checks failed

fongandrew commented 4 years ago

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 commented 4 years ago

how about simply modify the name to UNSAFE_xx ?

fongandrew commented 4 years ago

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

fongandrew commented 4 years ago

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

Chr1stian commented 4 years ago

@scniro can you please review and merge this?

scniro commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 💯