Skip to content
This repository has been archived by the owner on Oct 1, 2024. It is now read-only.

[react-testing] do not use Promise.race to release stale act promises #2441

Merged
merged 1 commit into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/green-hats-lie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/react-testing': patch
---

Fixed an issue with useLazyQuery failing due to act resolving a tick later
52 changes: 38 additions & 14 deletions packages/react-testing/src/root.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,7 @@ export class Root<Props> implements Node<Props> {
private root: Element<Props> | null = null;
private acting = false;
private destroyed = false;
private actPromise!: Promise<void>;
private actPromiseResolver!: () => void;
private actCallbacks: Set<() => void> = new Set();

private render: Render;
private resolveRoot: ResolveRoot;
Expand All @@ -91,9 +90,6 @@ export class Root<Props> implements Node<Props> {
) {
this.render = render;
this.resolveRoot = resolveRoot;
this.actPromise = new Promise((resolve) => {
this.actPromiseResolver = resolve;
});
this.mount();
}

Expand Down Expand Up @@ -140,10 +136,38 @@ export class Root<Props> implements Node<Props> {
return result as unknown as Promise<void>;
}

return Promise.race([
result,
this.actPromise,
]) as unknown as Promise<void>;
/* return a thenable which when invoked will add react's callback (to clear queue)
* into a set. If the result ever resolves we will remove the callback from the Set.
* If it doesn't we will call all callbacks in the Set when this root is destroyed which
* will unblock any stuck queues allowing subsequent tests to run
* Note: This can be cleanly achieved with a `Promise.race` but that adds an extra micro-task
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a comment about Promise.race in case anyone would be tempted to refactor this in the future 😓

Copy link
Member

Choose a reason for hiding this comment

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

A test case that demonstrates this picky behavior that fails if you try to refactor this to use Promise.race would be a fantastic follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good call, I'll see if I can put a test case together later. It's going to quite tricky to extract a minimal test case from that apollo-client mess

Copy link
Member

Choose a reason for hiding this comment

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

I've been also been dealing with what can broadly be described as "apollo-client mess where task/microtask ordering is picky" too, I am full of sympathy.

* which can potentially break fragile logic that depend on specific update timings.
* (eg. testing errors with useLazyQuery from apollo )
*/

return {
then: (callback, error) => {
this.actCallbacks.add(callback);
if (this.destroyed) {
return Promise.resolve();
}

return (result as unknown as Promise<void>).then(
(value) => {
this.actCallbacks.delete(callback);
if (!this.destroyed) {
return callback(value);
}
},
(value) => {
this.actCallbacks.delete(callback);
if (!this.destroyed) {
return error(value);
}
},
);
},
} as unknown as Promise<void>;
}

return undefined as unknown as Promise<void>;
Expand Down Expand Up @@ -274,21 +298,21 @@ export class Root<Props> implements Node<Props> {
}

this.ensureRoot();
this.act(() => this.reactRoot!.unmount());
return this.act(() => this.reactRoot!.unmount());
}

async destroy() {
const {element, mounted} = this;
let mountedPromise;

if (mounted) {
this.unmount();
mountedPromise = this.unmount();
}
element.remove();
connected.delete(this);
this.destroyed = true;
this.actPromiseResolver();
// flush the micro task to wait until react commits all pending updates.
await this.actPromise;
await mountedPromise;
this.actCallbacks.forEach((callback) => callback());
Copy link
Member

Choose a reason for hiding this comment

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

Won't these need to be awaited as well? And shouldn't they be invoked prior to unmounting?

Copy link
Contributor Author

@melnikov-s melnikov-s Oct 19, 2022

Choose a reason for hiding this comment

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

Won't these need to be awaited as well?

These callbacks will popActScope and recursivelyFlushAsyncActWork if needed. There's nothing to await there.

And shouldn't they be invoked prior to unmounting?

Once the root is destroyed there should be no more assertions on it, we're calling these to flush the queue out and it's not really related to this instance of the root itself. I tried moving them up but it fails the tests as it appears that the final act in the unmount needs to be called and awaited on first.

Copy link
Member

@BPScott BPScott Oct 19, 2022

Choose a reason for hiding this comment

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

I initially thought "should this be prior to the unmount" too. I was thinking there'd be an uptick in "you tried to act on a component that is unmounted" console warnings but that doesn't seem to be the case, thought admittedly the sheer noisyness of web's tests makes that hard to fully confirm.

}

setProps(props: Partial<Props>) {
Expand Down