-
-
Notifications
You must be signed in to change notification settings - Fork 2k
[New] add componentDidCatch support, and simulateError
#1797
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
Conversation
| "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
| }, | ||
| 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.
calinoracation
left a comment
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! |
| }; | ||
| }, | ||
| 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
.simulateErrorAPI to shallow and mount wrappers, along with support forcomponentDidCatch.Fixes #1255.