removing usage of componentWillReceiveProps

[saboya]

Removes usage of componentWillReceiveProps and uses getDerivedStateFromProps instead.

Haven't done a lot of testing on this but passes all tests and seems fine when randomly clicking around in the dev server.

Fixes these, and probably some others:
#3374
#3191
#2277

[saboya]

PS: This obviously implies that react version requirements would be >=16.3.0.

[ranneyd]

@JedWatson can we get the ball rolling on this? This warning is making me sad

[DougPuchalski]

This is why we try to minimize our dependency footprint. Have to either keep a library up to date or give it up for someone else to maintain. We'll probably abandon this and build our own unfortunately. I need libraries to be clean in the logs so I can find real app errors that need attention.

[saboya]

Rebased.

[ranneyd]

If someone forks this and publishes to npm I'll give them $10 and a GitHub star

[saboya]

If someone forks this and publishes to npm I'll give them $10 and a GitHub star

@ranneyd

There you go: https://www.npmjs.com/package/@saboya/react-select

Have no idea if this actually works correctly, but give it a shot.

[ranneyd]

LMAO let me try this out. Send me your venmo

[ranneyd]

@saboya
Good news: your npm package worked like a charm!
Bad news: there's still a deprecation message. It's only one out of the three and it's from this library:
https://github.com/JedWatson/react-input-autosize

[ranneyd]

@saboya does this fix look correct to you? If so, I'll merge and publish, and you can update your deps
https://github.com/ranneyd/react-input-autosize/pull/1

[saboya]

@ranneyd Just published version 3.0.5 for my package with a new dep, see if it works for you.

[ranneyd]

@saboya still getting the error

Use mine:

https://www.npmjs.com/package/react-input-autosize-temp

[JedWatson]

Thanks for the PR @saboya

We've now comprehensively fixed the warnings with new releases of react-select and its other dependencies so they should go away now.

In terms of merging this change, I'm not confident that it's tested sufficiently and don't think the refactor is worth the risk of potentially breaking people's usage of the component without a major version bump. We're looking into a refactor with hooks that should simplify the internal implementation considerably and that would be released as 4.0 through the same alpha / beta / stable release process we used for v2 so we can ensure minimal risk and impact on the community.

Up to you @ranneyd and @saboya if you want to continue maintaining those forks, we're currently merging a bunch of other fixes into master at the moment 🙂

[JedWatson]

p.s

some other context on why I'm wary of this from #3191

It looks like all uses of componentWillReceiveProps can be trivially replaced with componentDidUpdate, at the minor cost of potential double-renders when certain props change. Could use getDerivedStateFromProps instead, but that causes more problems than it's worth IMO.

This logic in the current version of react-select is really stable and I'm really hesitant to risk that unnecessarily. It takes a huge amount of work to safely maintain a component that gets over 1.6m downloads a week, and while there are a lot of issues and PRs that come in, literally millions of people use it without a problem and ensuring stability is a really high priority.

It can take up to half an hour and sometimes several people to review each issue and PR that comes in, even if they look simple, so it's a considerable labour of love maintaining this library. With all respect here, "haven't done a lot of testing but it seems fine when clicking around the dev server" isn't actually something I can merge without putting in a bunch of time making sure I've thought through and caught any unexpected implications of this kind of change.

Feedback on PRs like "I've reviewed this and it's safe because A B C, I've tested it thoroughly against use cases X Y Z" or "if we merge this and release it I'm happy to be on call to help triage and fix any issues it may cause" is the most helpful thing anyone can do for an open source project like this - because that's the responsibility the core maintainers take for this project.

Hope that provides some human insight into what's going on behind the scenes.

[ranneyd]

@JedWatson thanks for the thoughtful response. As far as "maintaining those forks" my plan was always to use a fork until an official patch was in. I've been using this library for a couple of years now and I swear by it, and I'm really appreciative of y'all making and maintaining it.

A few things I've learned about this issue since my original comments (which is a decent amount since this issue is pervasive across a ton of different open source projects out there) (also probably not relevant to you or this lib because the problems are already fixed, but for anyone who's watching this thread or stumbles upon this later):

• You can replace componentWillRecieveProps with UNSAFE_componentWillReceiveProps with absolutely no change to the functionality. The new name is just an alias, labeled "UNSAFE" not because it's actually inherently unsafe but because it's bug-prone and unadvisable. Doing a (careful) find-replace on that name makes the problem go away and should make absolutely no difference in functionality. It's a great option in lieu of a big refactor toward hooks/functional components.
• They released this alias in React 16.3, which came out March 2018. They said when they released it that componentWillRecieveProps will be completely removed from React in v17, but a console warning will start happening in 16.9 (now). Here's the original article. They specifically say "The primary purpose of the upcoming version 16.3 release is to enable open source project maintainers to update their libraries in advance of any deprecation warnings". That was 1.5 years ago, so it's a little disappointing this wasn't addressed until it was too late. That being said, this is far from the only library that didn't address this earlier (I'm looking at you AirBnB)
• The only caveat to using the UNSAFE versions is the lib becomes incompatible with React <16.3, because that's when the alias was introduced.

Thanks for responding and taking the issue seriously. I'll be switching back to the official lib tomorrow.

[saboya]

@JedWatson I appreciate the thoughtful response. I personally don't care too much if this gets merged or not, it's not my project and I can always maintain a fork if I want to, that's what open source is all about. I made this PR when I was working on a project with react-select, which is no longer the case.

Now, regarding my "testing": I've used this code in production at the time, and it worked fine, and I used a lot of features. Now, I must say "With all respect here", if the tests written for the package aren't enough to ensure correct functionality, I'm not sure what's going to be sufficient to get anything merged. This is a rather simple change and the logic itself should not be too complex to analyze, regardless of tests.

It's your project and your burden to maintain, and I respect the skepticism to ensure the users don't get a borked release, but I don't appreciate the attitude towards what I consider a meaningful contribution.

Thanks for your efforts maintaining this popular package.

[JedWatson]

Hey @saboya

My apologies for the way my comments sounded to you, and especially for the attitude you heard in my comments about testing. I truly did not intend for them to have that impact on you personally, or to reflect poorly on your contribution (which despite the conversation above or whether it gets merged, I truly do appreciate)

The additional context of using this code in production (and the number of feature you are using) does make a difference to how the PR is reviewed. That's what I meant to convey in my p.s.

In the hope of providing helpful feedback, the original description of the PR, specifically:

Haven't done a lot of testing on this but passes all tests and seems fine when randomly clicking around in the dev server.

... didn't give me the confidence to easily push this through without digging in further. I also meant to give anyone following or participating in this conversation the context of why it's not just "the maintainers clicking a few buttons" as was mentioned on another issue. But I didn't mean to undermine or dismiss your contribution itself, so again - I'm sorry that happened.

re: testing and confidence, we have extensive unit tests and cypress coverage, but there are still performance and caching mechanisms (or creative approaches to managing side effects, especially with regards to focus handling) that the core lifecycle methods currently handle in a very nuanced way. I'm sure they can be refactored safely. It's just also a specific area of the code that I'd want to think through and review very carefully to make sure the impact of changes are well understood.

In hindsight that could be called out better or be given as feedback earlier in the review process; I absolutely wear that point. Also @ranneyd's feedback about how we could have addressed this before it became a problem for users; that's fair.

You can replace componentWillRecieveProps with UNSAFE_componentWillReceiveProps with absolutely no change to the functionality.

This ended up being the safest approach, and makes sense given we're looking into using hooks to better encapsulate the complexity of react-select's behaviour, so that's what we've done for now.