✨ Add cleanup option to createMount in react-testing#2102
Conversation
BPScott
left a comment
BPScott
left a comment
There was a problem hiding this comment.
Looking great to me!
Remember the PR link in the changelog and 
| if (cleanup) { | ||
| const originalDestroy = wrapper.destroy.bind(wrapper); | ||
| wrapper.destroy = () => { | ||
| cleanup(wrapper, options); |
There was a problem hiding this comment.
Would it be a lot of effort to support async cleanup? A common pattern in web is "loading state tests need end with a graphQL.resolveAll() to complete pending React.act calls".
If we could automate that, adding an afterEach check for dangling acts becomes less noisy.
There was a problem hiding this comment.
My worry is that destroy is currently sync, which means that existing uses could break if its unexpectedly changed, and simply overwriting it with an async function in some cases doesn't fix types. I can take a crack at it as a follow-up though.
There was a problem hiding this comment.
also @GoodForOneFare if you have any ideas about how to deal with the points above (or think they're not a big deal) that's also good info :P
There was a problem hiding this comment.
Those are good points! I figured it wouldn't be trivial.
Overnight, I talked myself into wanting an alternative solution. Something more explicit, and part of general test flow.
The bad idea version of this looks like:
await component.trigger('onFocus', {andTotesResolveGraphQLForMe: true});
packages/react-testing/CHANGELOG.md
Outdated
|
|
||
| ### Added | ||
|
|
||
| - Added new `cleanup` option to `createMount` |
There was a problem hiding this comment.
:yells-at-pr-self-reference:
GoodForOneFare
left a comment
GoodForOneFare
left a comment
There was a problem hiding this comment.
👍 I have a question about async support, but this can ship as is.
Co-authored-by: Gord P <GoodForOneFare@users.noreply.github.com>
5 participants
Description
This PR adds a
cleanupfunction tocreateMountin@shopify/react-testingthat can be used to handle any additional cleanup a wrapper or its context might need. This was originally proposed by @GoodForOneFare for stopping any ongoing gql requests and other such async handlers that may be leaking out of our context objects.Type of change
@shopify/react-testingMinor: New feature (non-breaking change which adds functionality)Checklist