-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
[New] add componentDidCatch
support, and simulateError
#1797
Conversation
package.json
Outdated
@@ -90,6 +90,7 @@ | |||
"gitbook-plugin-github": "^2.0.0", | |||
"in-publish": "^2.0.0", | |||
"istanbul": "^1.0.0-alpha.2", | |||
"istanbul-api": "~1.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a dep of istanbul itself; see istanbuljs/istanbuljs#216
@@ -262,6 +263,19 @@ class ReactSixteenOneAdapter extends EnzymeAdapter { | |||
getNode() { | |||
return instance ? toTree(instance._reactInternalFiber).rendered : null; | |||
}, | |||
simulateError(nodeHierarchy, rootNode, error) { | |||
const { instance: catchingInstance } = nodeHierarchy | |||
.find(x => x.instance && x.instance.componentDidCatch) || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I have multiple error boundary components in my tree, will this throw on the closest one to the current node?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should throw on the first one it finds as it traverses upwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great! Will this PR need to update the API documentation as well?
describeIf(is('>= 16'), 'componentDidCatch', () => { | ||
describe('errors inside an error boundary', () => { | ||
const errorToThrow = new EvalError('threw an error!'); | ||
// in React 16.0 - 16.2, and sole older nodes, the actual error thrown isn't reported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully understand this comment: in React 16.0 - 16.2, and sole older nodes
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, typo for “some”
render() { | ||
const { throws } = this.state; | ||
return ( | ||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the span
nested in the div
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure that the hierarchy logic works properly :-)
getDisplayName, | ||
); | ||
|
||
componentDidCatch.call(catchingInstance, error, { componentStack }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is componentStack
an object here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React passes an “info” object with a componentStack property
render() { | ||
const { throws } = this.state; | ||
return ( | ||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the div
and span
here needed / helpful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the more common pattern that we use would be along the lines of:
return (
<React.Fragment>
<Thrower throws={throws} />
</React.Fragment>
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but by adding them, i was able to find bugs in the hierarchy logic.
I’ll add another test for fragments.
|
||
expect(spy.args).to.be.an('array').and.have.lengthOf(1); | ||
const [[actualError, info]] = spy.args; | ||
expect(() => { throw actualError; }).to.throw(errorToThrow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you use a different syntax to test between this one and the mount
test, whereas here you rethrow the actualError
but don't in the mount one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, it checks the exception type and the message.
I can't do that in ReactWrapper, unfortunately, because the error thrown is replaced in some nodes and some Reacts :-/
e408e68
to
e1f596c
Compare
@calinoracation thanks for the reminder on the docs; I've added some. |
e1f596c
to
936abdb
Compare
|
||
render() { | ||
const { children } = this.props; | ||
const { throws } = this.state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
|
||
render() { | ||
const { children } = this.props; | ||
const { throws } = this.state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
f9cd6e0
to
1626578
Compare
1626578
to
944f9e0
Compare
- [new] add `simulateError` to `shallow` and `mount` renderers (#1797)
- [new] add `simulateError` to `shallow` and `mount` renderers (#1797)
- [new] add `simulateError` (#1797)
This is great! I was just trying to write a test for an error boundary and this made it possible. Thank you! |
@@ -327,6 +342,16 @@ class ReactSixteenOneAdapter extends EnzymeAdapter { | |||
: elementToTree(output), | |||
}; | |||
}, | |||
simulateError(nodeHierarchy, rootNode, error) { | |||
simulateError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not check the nodeHierarchy
for error boundaries in a shallow renderer? Should this scenario require a mount renderer?
// ErrorBoundary is a component with a componentDidCatch method that renders null
// if an error is encountered.
function MyComponent({children}) {
return (
<div>
<h3>My component is cool!</h3>
<ErrorBoundary>
{children}
</ErrorBoundary>
</div>
);
}
const BadChild = () => null;
const wrapper = shallow(
<MyComponent>
<BadChild />
</MyComponent>
);
wrapper.find(BadChild).simulateError(new Error('That was bad'));
expect(wrapper.find('h3')).toExist();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a shallow render, there can't ever be any other error boundaries, since only the root node is actually rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but if simulateError
is called on an element in shallow render tree, it seems reasonable to render any error boundaries found above that element.
This actually seems to work:
const wrapper = shallow(
<MyComponent>
<BadChild />
</MyComponent>
);
const errorBoundary = wrapper.find('ErrorBoundary').shallow();
errorBoundary.find(BadChild).simulateError(new Error('That was bad'));
expect(wrapper.find('h3')).toExist();
...but it doesn't really feel intuitive. I can see people trying my previous example and being confused when it doesn't work, especially since I did just that :\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not reasonable or possible, because of how shallow rendering works. In shallow rendering, everything that's not the root node is treated as if it's a div - ie, as if the component implementation is ({ children }) => children
. Thus, there's no componentDidCatch
anywhere, except the top.
This PR adds a
.simulateError
API to shallow and mount wrappers, along with support forcomponentDidCatch
.Fixes #1255.