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.

Sign up for GitHub

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

Jump to bottom

Support getDerivedStateFromError #2036

Merged
merged 3 commits into from Mar 19, 2019
Merged

Support getDerivedStateFromError #2036

merged 3 commits into from Mar 19, 2019

Conversation

barmac
Copy link
Contributor

@barmac barmac commented Mar 6, 2019

This PR adds support for static getDerivedStateFromError.

@ljharb ljharb added this to Lifecycles in React 16 Mar 7, 2019
ljharb
ljharb requested changes Mar 7, 2019
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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 Show resolved Hide resolved
@ljharb ljharb mentioned this pull request Mar 7, 2019
Open
41 tasks
@barmac
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
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.

@ljharb
Copy link
Member

ljharb commented Mar 12, 2019 •
edited

@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)

@barmac
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.

@ljharb
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.

ljharb
ljharb requested changes Mar 13, 2019
View reviewed changes
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 Show resolved Hide resolved
@barmac
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
Copy link
Contributor Author

barmac commented Mar 13, 2019 •
edited

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

ljharb
ljharb requested changes Mar 15, 2019
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, almost there :-)

packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js Outdated Show resolved Hide resolved
packages/enzyme-adapter-utils/src/Utils.js Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ReactWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
packages/enzyme-test-suite/test/ShallowWrapper-spec.jsx Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Mar 16, 2019

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

@barmac
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
Copy link
Member

ljharb commented Mar 17, 2019 •
edited

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.

@barmac
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
Copy link
Member

ljharb commented Mar 19, 2019

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

@ljharb
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 merged commit dfd1289 into enzymejs:master Mar 19, 2019
ljharb added a commit that referenced this pull request Apr 6, 2019
@ljharb
[enzyme-adapter-utils] v1.11.0
2c88066 
 - [new] `Utils` add `findElement`, `getNodeFromRootFinder`, `wrapWithWrappingComponent`, `getWrappingComponentMountRenderer`; add `RootFinder` (#1966)
 - [new] add `getDerivedStateFromError` support (#2036)
 - [fix] `shallow`/`mount`: `renderProp`: avoid warning when render prop returns `null` (#2076)
 - [build] include source maps
 - [deps] update `eslint`
 - [dev deps] update `eslint`, `semver`, `rimraf`, `react-is`, `html-element-map`, `chai`, `eslint-plugin-mocha`
ljharb added a commit that referenced this pull request Apr 6, 2019
@ljharb
[enzyme-adapter-react-16] v1.12.0
3a2f5ba 
 - [new] Add support for wrapping `Profiler` (#2055)
 - [new] support shallow rendering `createContext()` providers and consumers  - add `isContextConsumer`, `getProviderFromConsumer` (#1966)
 - [new] add `wrapWithWrappingComponent`, `isCustomComponent` (#1960)
 - [new] add `getDerivedStateFromError` support (#2036)
 - [fix] avoid invariant violation in provider (#2083)
 - [fix] properly fix finding memo(SFC) components (#2081)
 - [fix] properly render memoized SFCs
 - [fix] `shallow`: avoid wrapping component for hooks
 - [deps] update `react-is`
 - [dev deps] update `eslint`
 - [refactor] use `react-is` predicates more
 - [build] include source maps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
package: enzyme-adapter-utils package: react adapter: 16 semver: minor New stuff.
Projects
React 16
  
Lifecycles
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@barmac @ljharb