Fix waiter left behind when resource is canceled before promise resolves

josemarluedke · Sep 29, 2023 · 47dac13 · 47dac13
1 parent f3de5dd
commit 47dac13
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 9 deletions.
diff --git a/packages/glimmer-apollo/src/-private/query.ts b/packages/glimmer-apollo/src/-private/query.ts
@@ -31,16 +31,16 @@ export interface QueryOptions<TData, TVariables extends OperationVariables>
 export type QueryPositionalArgs<
   TData,
   TVariables extends OperationVariables = OperationVariables
-> = [DocumentNode, QueryOptions<TData, TVariables>?];
+  > = [DocumentNode, QueryOptions<TData, TVariables>?];
 
 export class QueryResource<
   TData,
   TVariables extends OperationVariables = OperationVariables
-> extends ObservableResource<
+  > extends ObservableResource<
   TData,
   TVariables,
   TemplateArgs<QueryPositionalArgs<TData, TVariables>>
-> {
+  > {
   @tracked loading = true;
   @tracked error?: ApolloError;
   @tracked data: TData | undefined;
@@ -50,6 +50,8 @@ export class QueryResource<
   #subscription?: ObservableSubscription;
   #previousPositionalArgs: typeof this.args.positional | undefined;
 
+  #firstPromiseReject: Function | undefined;
+
   /** @internal */
   async setup(): Promise<void> {
     this.#previousPositionalArgs = this.args.positional;
@@ -64,6 +66,7 @@ export class QueryResource<
     }
 
     let [promise, firstResolve, firstReject] = createPromise(); // eslint-disable-line prefer-const
+    this.#firstPromiseReject = firstReject;
     this.promise = promise;
     const observable = client.watchQuery({
       query,
@@ -82,9 +85,9 @@ export class QueryResource<
       },
       (error) => {
         this.#onError(error);
-        if (firstReject) {
-          firstReject();
-          firstReject = undefined;
+        if (this.#firstPromiseReject) {
+          this.#firstPromiseReject();
+          this.#firstPromiseReject = undefined;
         }
       }
     );
@@ -112,6 +115,10 @@ export class QueryResource<
     if (this.#subscription) {
       this.#subscription.unsubscribe();
     }
+    if (this.#firstPromiseReject) {
+      this.#firstPromiseReject();
+      this.#firstPromiseReject = undefined;
+    }
   }
 
   settled(): Promise<void> {

diff --git a/packages/test-app/app/components/playground/experiment.hbs b/packages/test-app/app/components/playground/experiment.hbs
@@ -1,4 +1,4 @@
-<h2>
+<h2 data-test-id="experiment">
   User Info
 </h2>
 

diff --git a/packages/test-app/app/components/playground/experiment.ts b/packages/test-app/app/components/playground/experiment.ts
@@ -23,6 +23,7 @@ export default class PlaygroundExperiment extends Component {
   userInfo = useQuery(this, () => [
     USER_INFO,
     {
+      variables: { id: '1-with-delay' },
       errorPolicy: 'all',
       notifyOnNetworkStatusChange: true
     }

diff --git a/packages/test-app/app/components/playground/index.hbs b/packages/test-app/app/components/playground/index.hbs
@@ -1,4 +1,4 @@
-<button type="button" {{on "click" this.toggle}}>
+<button type="button" {{on "click" this.toggle}} dates-test-id="toggle">
   Toggle Experiment
 </button>
 

diff --git a/packages/test-app/app/components/playground/index.ts b/packages/test-app/app/components/playground/index.ts
@@ -2,7 +2,7 @@ import Component from '@glimmer/component';
 import { tracked } from '@glimmer/tracking';
 
 export default class Playground extends Component {
-  @tracked isExperimenting = true;
+  @tracked isExperimenting = false;
 
   toggle = (): void => {
     this.isExperimenting = !this.isExperimenting;

diff --git a/packages/test-app/app/mocks/handlers.ts b/packages/test-app/app/mocks/handlers.ts
@@ -84,6 +84,16 @@ export const handlers = [
           }
         })
       );
+    } else if (user && req.variables.id.includes('with-delay')) {
+      return res(
+        ctx.delay(300),
+        ctx.data({
+          user: {
+            __typename: 'User',
+            ...user
+          }
+        })
+      );
     } else if (user) {
       return res(
         ctx.data({

diff --git a/packages/test-app/tests/integration/components/playground-test.ts b/packages/test-app/tests/integration/components/playground-test.ts
@@ -0,0 +1,18 @@
+import { hbs } from 'ember-cli-htmlbars';
+import { module, test } from 'qunit';
+import { setupRenderingTest } from 'ember-qunit';
+import { click, render, waitFor } from '@ember/test-helpers';
+
+module('Integration | Components | Playground', function(hooks) {
+  setupRenderingTest(hooks);
+
+  test('no waiters should be left behind when resource is teardown before resolving', async function(this, assert) {
+    await render(hbs`<Playground />`);
+
+    click('[dates-test-id="toggle"]');
+    await waitFor('[data-test-id="experiment"]', { timeout: 100 });
+
+    await click('[dates-test-id="toggle"]');
+    assert.ok(true);
+  });
+});