Doesn't demand `null` be returned

[catamphetamine]

The current implementation of getDerivedStateFromProps() polyfill is not fully correct: it doesn't demand null be returned which is required by the official spec.

function componentWillReceiveProps(nextProps) {
  // Call this.constructor.gDSFP to support sub-classes.
  var state = this.constructor.getDerivedStateFromProps(nextProps, this.state);
  if (state !== null && state !== undefined) {
    this.setState(state);
  }
}

Instead it should be:

  ...
  if (state !== null && state !== undefined) {
    this.setState(state);
  }
  if (state === undefined) {
    console.warn('Warning: getDerivedStateFromProps(): A valid state object (or null) must be returned. You have returned undefined.');
  }
}

[bvaughn]

This raises an interesting question. How closely should the polyfill mirror future releases of React in terms of DEV-mode warnings?

I'm not convinced this is necessary, for a couple of reasons:

1. If we changed the wording of this warning in future versions- which version would the polyfill mirror?
2. More importantly, this polyfill exists for library authors- in order to enable them to support older versions of React. However, they should definitely be testing with newer versions as well- and since this warning exists there, they will see it there.

[catamphetamine]

How closely should the polyfill mirror future releases of React in terms of DEV-mode warnings?

I'd say as close as possible. Isn't it the sole purpose of a "polyfill".

If we changed the wording of this warning in future versions- which version would the polyfill mirror?

I'd say the polyfill would need to adapt to the latest version. Or release a new breaking version of itself, though I don't think changing warning's text would break anyone's code.

More importantly, this polyfill exists for library authors- in order to enable them to support older versions of React. However, they should definitely be testing with newer versions as well- and since this warning exists there, they will see it there.

I'm not testing with new versions of React.
And I shouldn't.
If you tell your polyfill works - then I take your word for it.
If it doesn't, or you don't guarantee it - well, you better unpublish it from npm and stop deceiving people into bringing a ticking bomb to their favourite libraries.
Are you telling that the task of writing a polyfill for the new lifecycles is non-deterministic and can't have a 100% mathematically correct solution?
I don't think so.

[bvaughn]

Let's leave this issue open a bit and see what others think. I don't feel very strongly about this.

I'd say as close as possible. Isn't it the sole purpose of a "polyfill".

The purpose of this polyfill, as mentioned in the README, is to enable library authors to avoid forking their libraries for React < 16.3 and >= 16.3.

DEV warnings are not required to do this. 😄

I'm not testing with new versions of React.
And I shouldn't.

I believe you misunderstand me.

Let's say I'm a library author and I want to use the new React lifecycles so that I can remove the deprecated ones before my users see any deprecation warnings.

I trust that I can use these new lifecycles and this polyfill will work the same for older and newer versions of React- so people can continue to use newer my library regardless of whether they're using React 15 or 16, etc. (This is true.)

This doesn't mean that I shouldn't also test my library with a newer version of React to make sure nothing else has changed or been deprecated. While doing this testing- if I return undefined from the new lifecycle- I'd see a warning. This is all I meant.

The lifecycle exists to provide library maintainers more flexibility. It does not remove the necessity of testing. 😄

[catamphetamine]

The purpose of this polyfill, as mentioned in the README, is to enable library authors to avoid forking their libraries for React < 16.3 and >= 16.3.

Yeah, and that's a good thing.

DEV warnings are not required to do this.

Well, strictly speaking it doesn't affect operation, but it would enable developers catch these "doesn't return null" bugs earlier, instead of reacting when someone posts an issue saying that there's a warning.

This doesn't mean that I shouldn't also test my library with a newer version of React to make sure nothing else has changed or been deprecated. While doing this testing- if I return undefined from the new lifecycle- I'd see a warning. This is all I meant.

Ok.

I don't feel very strongly about this.

You aren't ought to.

[klujanrosas]

The polyfill should match the "real thing" as close as possible IMHO, however I do agree with @bvaughn on the fact that if the wording changed in future versions there would be some kind of confusion as to what version this polyfill would be targeting/supporting.

I'm sure library(at least the most used ones) authors are for sure testing against newer versions, but a regular Joe trying out the polyfill might not actually go ahead and test against the newest version, therefore not knowing about the warning.

I feel like displaying a warning does not add benefit to people experienced in React, but it does for those who are maybe exploring the waters and either didn't read the docs or aren't that much familiarized with React overall.