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

Update to match 16.4 gDSFP semantics #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

acdlite
Copy link
Member

@acdlite acdlite commented May 29, 2018

Conceptually, getDerivedStateFromProps is like a setState updater function. But it needs to be called after all the other updates in the queue, and it must be called exactly once, right before shouldComponentUpdate.

Other than getDerivedStateFromProps itself, there's no lifecycle that meets these requirements. componentWillReceiveProps fires only if the props have changed. componentWillUpdate fires too late.

This polyfill works by monkeypatching instance.updater. updater is an internal-ish API that has stayed mostly stable across several major versions of React. The polyfill intercepts updates before they are added to the native update queue. Then it schedules a single update with the native update queue, in the form of an updater function. Inside that updater function, the intercepted updates are processed. Finally, getDerivedStateFromProps is called and applied. This approach guarantees that getDerivedStateFromProps is always called at the end.

Note: The polyfill does not attempt to fix React 16.3. Components running in 16.3 will use the native, incorrect behavior. Consumers should upgrade to 16.4.

Unresolved questions:

  • Shallow renderer support. The shallow renderer uses instance.updater in non-standard ways. As far as I can tell, there's no way to polyfill getDerivedStateFromProps correctly without relying on shallow renderer specific internals like updater._invokeCallbacks and instance._newState.
  • Limited support for forceUpdate. Because forceUpdate does not accept a setState updater function, getDerivedStateFromProps will not be called as a result of forceUpdate, unless it's part of the same batch as a setState or an update on a parent component.

@bvaughn
Copy link
Contributor

bvaughn commented May 30, 2018

I'm not sure the benefits of this PR outweigh the costs.

Positives:

  • More faithful polyfill for 16.4+

Negatives:

  • Loss of support for e.g. Preact, Inferno.
  • Increased size and complexity (which may encourage size-conscious people to aim for a cheaper inline polyfill).
  • No way to avoid subtle incompatibilities between 16.3 and other versions. (Although to be fair, some form of incompatibility will exist regardless.)

@markerikson
Copy link

It seems silly to have a polyfill that does not match the "correct" semantics. We are trying to use this with React-Redux 5.1, and we've got a branch with tests that are passing with 16.4 but failing with <= 16.3.

@eternalsky
Copy link

how is this pr going on ? it's really confusing that we need to fix some bugs lower than 16.4 when all tests are passed in 16.4.x.

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