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

Support getDerivedStateFromError #2036

Merged
merged 3 commits into from Mar 19, 2019

Conversation

2 participants
@barmac
Copy link
Contributor

barmac commented Mar 6, 2019

This PR adds support for static getDerivedStateFromError.

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch from 067f7c3 to 303c1f2 Mar 6, 2019

@ljharb ljharb added this to Lifecycles in React 16 Mar 7, 2019

@ljharb
Copy link
Member

ljharb left a comment

Thanks, this is great!

Could we also add a test (to both shallow and mount) that has both gDSFE and cDC, and asserts on their relative orderings?

Show resolved Hide resolved docs/api/ReactWrapper/simulateError.md Outdated
Show resolved Hide resolved docs/api/ReactWrapper/simulateError.md
Show resolved Hide resolved docs/api/ReactWrapper/simulateError.md Outdated
Show resolved Hide resolved docs/api/ShallowWrapper/simulateError.md Outdated
Show resolved Hide resolved docs/api/ShallowWrapper/simulateError.md
Show resolved Hide resolved docs/api/ShallowWrapper/simulateError.md Outdated
Show resolved Hide resolved packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
Show resolved Hide resolved packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
Show resolved Hide resolved packages/enzyme-adapter-utils/src/Utils.js Outdated
Show resolved Hide resolved packages/enzyme-adapter-utils/src/Utils.js Outdated
@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 7, 2019

Thanks for the feedback! I'll apply the changes/resolve conversations today evening or on Friday (UTC+1).

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch 5 times, most recently from 123a07e to 838640b Mar 11, 2019

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch 6 times, most recently from 51d4980 to e0a0f53 Mar 11, 2019

@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 11, 2019

OK, so the tests revealed that simulateError does not cause an error boundary failure which can be triggered via real error during render.

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch from e0a0f53 to 9e9f8cf Mar 11, 2019

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 12, 2019

@barmac can you elaborate on that last comment? simulateError is intended to be able to cause the same behavior as an error during actual rendering. If it doesn't, it may need to be fixed.

(also, i fixed your use of mount in the shallow tests, and now there's a bunch of other failures)

@ljharb ljharb force-pushed the barmac:support-get-derived-state-from-error branch from 9e9f8cf to 3c96e05 Mar 12, 2019

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch 3 times, most recently from 30bcce9 to 1086a3f Mar 12, 2019

@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 12, 2019

Thanks for the shallow fix.

I updated PR so that we actually pass the type to simulate error instead of relying on constructor property.

The previous comment is irrelevant as I forgot that with simulateError the component's state itself is not the cause of error, so even without state update the component should be able to rerender and be stable again. This is not the case regarding the real errors, where usually it is the component's state which is broken and not updating it forces React to unmount the whole tree.

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch from 1086a3f to b1668ff Mar 12, 2019

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 13, 2019

I'm not sure that's necessarily true; you could easily have a rendering error without corrupting the state.

Show resolved Hide resolved docs/api/ShallowWrapper/simulateError.md Outdated
Show resolved Hide resolved docs/api/ReactWrapper/simulateError.md Outdated
Show resolved Hide resolved packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
Show resolved Hide resolved packages/enzyme-adapter-react-16.1/src/ReactSixteenOneAdapter.js Outdated
Show resolved Hide resolved packages/enzyme-adapter-utils/src/Utils.js Outdated
Show resolved Hide resolved packages/enzyme-adapter-utils/src/Utils.js Outdated

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch from 5ae9a93 to 39f4e9b Mar 13, 2019

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch from 39f4e9b to 3177f98 Mar 13, 2019

@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 13, 2019

Rolled back react-adapter-16.1-3 changes as getDerivedStateFromError was introduced in React@16.6.

@barmac barmac force-pushed the barmac:support-get-derived-state-from-error branch from 3177f98 to 13c825f Mar 13, 2019

@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 13, 2019

Updated branch. @ljharb please check whether it's ready to be merged :).

@ljharb
Copy link
Member

ljharb left a comment

Thanks, almost there :-)

@ljharb ljharb force-pushed the barmac:support-get-derived-state-from-error branch from 13c825f to 8f3c967 Mar 16, 2019

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 16, 2019

I've made a few of the changes, and rebased; some tests are failing locally.

@ljharb ljharb force-pushed the barmac:support-get-derived-state-from-error branch from 8f3c967 to 8741943 Mar 16, 2019

@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 17, 2019

Thanks :)

I ran test:all on clean install and did not get any test failures. Are you sure this is not related to any files left after previous work?

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 17, 2019

Looks like react v16.8.3 fails, and v16.8.4 passes. I'll update master with a broader test matrix, and rebase this PR.

@ljharb ljharb force-pushed the barmac:support-get-derived-state-from-error branch from 8741943 to 068992a Mar 18, 2019

@barmac

This comment has been minimized.

Copy link
Contributor Author

barmac commented Mar 18, 2019

Still same, npm run react 16.8.3 && npm run test:only is all green. I confirmed with node_modules/react that I'm testing with the right version.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 19, 2019

hmm, interesting. maybe it's the test renderer? I'll check again.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Mar 19, 2019

k, now i can't reproduce :-) i'll rebase this, and once tests pass, merge. Thanks for your patience!

@ljharb ljharb force-pushed the barmac:support-get-derived-state-from-error branch from 068992a to dfd1289 Mar 19, 2019

@ljharb

ljharb approved these changes Mar 19, 2019

@ljharb ljharb merged commit dfd1289 into airbnb:master Mar 19, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.