[react-testing] Release act promises when root wrapper is destroyed

[melnikov-s]

Description

With React 18 a hanging promises returned from act can prevent other updates from occurring and since that happens on a global scope it will affect subsequent tests within the same test suite. This PR attempts to free up the act queue by resolving any unfulfilled promises when the root wrapper is destroyed.

Within our internal tests we typically have destroyAll within a afterEach which loops through all active wrappers and calls destroy on them. The change with this PR is that now destoryAll will have to be awaited so that any active promises holding up the act can be resolved.

This PR also adds a warning when attempting to call wrapper.act on an already destroyed root, this can happen if for example a graphQL context is shared between wrappers. Eventually we'd want to clean those up.

Note: this also eliminates the need to have something like

await act(async () => {
    await flushPromises();
  });

in a afterEach

[BPScott]

The change with this PR is that now destoryAll will have to be awaited so that any active promises holding up the act can be resolved.

This is a breaking change that is a major version bump as it means consumers of this package will have to change their behaviour.

[melnikov-s]

This is a breaking change that is a major version bump as it means consumers of this package will have to change their behaviour.

That's only if you want to take advantage of the auto-resolve, otherwise it should have no effect. I've verified this on subsets of web and all of SFN but I'll run the entire web suite tonight to verify that everything continues to pass with this commit and no additional changes.

[melnikov-s]

/snapit

[github-actions]

🫰✨ Thanks @melnikov-s! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/mime-types@0.0.0-snapshot-20220822175908

yarn add @shopify/react-app-bridge-universal-provider@0.0.0-snapshot-20220822175908

yarn add @shopify/react-async@0.0.0-snapshot-20220822175908

yarn add @shopify/react-cookie@0.0.0-snapshot-20220822175908

yarn add @shopify/react-csrf-universal-provider@0.0.0-snapshot-20220822175908

yarn add @shopify/react-form@0.0.0-snapshot-20220822175908

yarn add @shopify/react-google-analytics@0.0.0-snapshot-20220822175908

yarn add @shopify/react-graphql@0.0.0-snapshot-20220822175908

yarn add @shopify/react-graphql-universal-provider@0.0.0-snapshot-20220822175908

yarn add @shopify/react-html@0.0.0-snapshot-20220822175908

yarn add @shopify/react-hydrate@0.0.0-snapshot-20220822175908

yarn add @shopify/react-i18n@0.0.0-snapshot-20220822175908

yarn add @shopify/react-i18n-universal-provider@0.0.0-snapshot-20220822175908

yarn add @shopify/react-import-remote@0.0.0-snapshot-20220822175908

yarn add @shopify/react-network@0.0.0-snapshot-20220822175908

yarn add @shopify/react-router@0.0.0-snapshot-20220822175908

yarn add @shopify/react-server@0.0.0-snapshot-20220822175908

yarn add @shopify/react-testing@0.0.0-snapshot-20220822175908

yarn add @shopify/react-tracking-pixel@0.0.0-snapshot-20220822175908

yarn add @shopify/react-universal-provider@0.0.0-snapshot-20220822175908

yarn add @shopify/react-web-worker@0.0.0-snapshot-20220822175908

[melnikov-s]

Sorry for the delay, I ran these changes against web and everything passes without any additional changes in web. CI run here: https://buildkite.com/shopify/web-ci-builder/builds/522692 . I think we can keep it as a minor version bump.

[BPScott]

That buildkite you linked me to contains CI errors. There is a type-check error saying:

tests/modern/mount.tsx:303:5 - error TS2322: Type '() => void' is not assignable to type '() => Promise<void>'.
--
  | Type 'void' is not assignable to type 'Promise<void>'.
  |  
  | 303     root.destroy = () => {
  | ~~~~~~~~~~~~

Which is because this PR changes the return type of root.destroy so that it now returns a promise - the type has changed from () => void to () => Promise<void>. It also changes the type of destroyAll() so that it now returns a promise (as it returns promise.all(...), thus it doesn't do anything unless you you change your existing code that does destroyAll(), to await destroyAll().

This PR requires consumers to go update their test mounting code in the cases where they call destroyAll() (as it isn't clear if items will be destroyed) or if they adjust the destroy callback.

[melnikov-s]

I'm not sure if type changes constitutes a major version bump, if they do then yes this should be one but otherwise this should not affect runtime behaviour. destroyAll should be awaited but if it isn't it will continue to function as it did previously as demonstrated with the passing tests in that CI build.

Either way, I can make this a major version bump if preferred.

[ryanwilsonperkin]

Excellent work, thank you for digging into this and I'm sorry it took me so long to get back to you with a review. I've left some comments here about the approach taken, I'm inclined to agree with @BPScott too that this should be a major release, since many folks will have a synchronous call to destroy in their cleanup code which would now be broken without an await.

[ryanwilsonperkin]

This should throw an error instead of logging a message, we don't want to support calling act on a root after it has been destroyed

[melnikov-s]

I could do that, especially with the major version bump but it will be a Herculean effort to upgrade web. These are mostly caused by having shared test graphql context, which many tests do. The shared graphql context also causes the "act within an act" warning and in certain situations will time out a test.

So for sure something we want to clean up but I was thinking we have it as a warning initially so that they can be fixed by the respective product team.

[ryanwilsonperkin]

Why would the shared graphql context cause this behaviour? We've seen shared graphql context lead to issues before, but it shouldn't be causing an act to be performed on a previously destroyed wrapper.

[melnikov-s]

The typical aftertMount callback in createMount has this line:

graphqlClient.wrap((perform) => root.act(perform));

this will register a wrapper function on the graphql context, that function will be added to a list of functions and called inside-out. If the graphql context is shared it will end up calling root.act from previous tests that have already been destroyed.

[ryanwilsonperkin]

Ah! Okay I see what you mean, that seems like an unfortunate limitation of the graphql-testing library, it'd be nice for us to have something like an "unwrap" functionality as well that could be configured to remove those wrappers, or turn them into no-ops here. We can use the warning for now, but I suspect we'll want another major release in the near future that turns that to an explicit error and encourages a pattern for not reusing root in graphql testing

[melnikov-s]

Yeah good call! An unwrap on graphql-testing with an additional beforeUnmount lifecycle on createMount will do the trick here

[ryanwilsonperkin]

What purpose is the destroyed boolean serving that we don't currently get with the withRoot behaviour? It seems that whenever destroyed gets set to true, root will also be made unavailable, so we should be able to rely on that existing pattern. In most (all?) cases, act is already prefaced with a withRoot check

[melnikov-s]

This is mostly for backwards compatibility. I wanted existing code which relied on being able to perform act on a destroyed root (see reply above) to function as it did before.

As for re-using withRoot the issue with that is this.root being null is set as both the initial value and when it is destroyed and I need to make a distinction between un-initialized vs destroyed thus the new property.

[ryanwilsonperkin]

This should also call out that destroy is now async (not just destroyAll) since it's also a public API of Root