fix: finish all tasks before calling onSuccess callback

docs/generated/changelog.html

@@ -11,6 +11,12 @@ <h1>Agent-JS Changelog</h1>
 
     <section>
       <h2>Version x.x.x</h2>
+      <ul>
+        <li>
+          fix: finish all tasks before calling onSuccess auth callback in @dfinity/auth-client
+        </li>
+      </ul>
+      <h2>Version 0.15.6</h2>
       <ul>
         <li>feat: retry failed `read_state` requests</li>
       </ul>

packages/auth-client/src/index.test.ts

@@ -529,47 +529,31 @@ describe('Auth Client login', () => {
     expect(idpWindow.close).toBeCalled();
   });
 
-  it('should call onError if recieved an invalid success message', async () => {
+  it('should call onError if recieved an invalid success message', done => {
     setup({
       onAuthRequest: () => {
         idpMock.send({
           kind: 'authorize-client-success',
         });
       },
     });
-    const client = await AuthClient.create();
-    const failureFunc = jest.fn();
-    await client.login({ onError: failureFunc });
 
-    idpMock.ready();
-
-    expect(failureFunc).toBeCalled();
-    expect(idpWindow.close).toBeCalled();
-    client.logout();
-  });
-
-  it('should call onError in an async pattern', async () => {
-    setup({
-      onAuthRequest: () => {
-        idpMock.send({
-          kind: 'authorize-client-success',
-        });
-      },
-    });
-    const client = await AuthClient.create();
-    const cb = jest.fn();
-    const failureFunc = async () => {
-      await cb();
-    };
-    await client.login({ onError: failureFunc });
+    AuthClient.create()
+      .then(client => {
+        const onError = () => {
+          expect(idpWindow.close).toBeCalled();
 
-    idpMock.ready();
+          client.logout().then(done);
+        };
 
-    expect(cb).toBeCalled();
-    expect(idpWindow.close).toBeCalled();
+        return client.login({ onError: onError });
+      })
+      .then(() => {
+        idpMock.ready();
+      });
   });
 
-  it('should call onSuccess if recieved a valid success message', async () => {
+  it('should call onSuccess if recieved a valid success message', done => {
     setup({
       onAuthRequest: () => {
         // Send a valid request.
@@ -589,15 +573,19 @@ describe('Auth Client login', () => {
       },
     });
 
-    const client = await AuthClient.create();
-    const onSuccess = jest.fn();
-    client.login({ onSuccess: onSuccess });
+    AuthClient.create()
+      .then(client => {
+        const onSuccess = () => {
+          expect(idpWindow.close).toBeCalled();
 
-    idpMock.ready();
+          client.logout().then(done);
+        };
 
-    expect(onSuccess).toBeCalled();
-    expect(idpWindow.close).toBeCalled();
-    await client.logout();
+        return client.login({ onSuccess: onSuccess });
+      })
+      .then(() => {
+        idpMock.ready();
+      });
   });
 
   it('should call onError if the user closed the IDP window', async () => {
@@ -614,38 +602,6 @@ describe('Auth Client login', () => {
       ),
     ).rejects.toMatch(ERROR_USER_INTERRUPT);
   });
-  it('should call an async onSuccess if recieved a valid success message', async () => {
-    setup({
-      onAuthRequest: () => {
-        // Send a valid request.
-        idpMock.send({
-          kind: 'authorize-client-success',
-          delegations: [
-            {
-              delegation: {
-                pubkey: Uint8Array.from([]),
-                expiration: BigInt(0),
-              },
-              signature: Uint8Array.from([]),
-            },
-          ],
-          userPublicKey: Uint8Array.from([]),
-        });
-      },
-    });
-
-    const client = await AuthClient.create();
-    const cb = jest.fn();
-    const onSuccess = async () => {
-      cb();
-    };
-    await client.login({ onSuccess: onSuccess });
-
-    idpMock.ready();
-
-    expect(cb).toBeCalled();
-    expect(idpWindow.close).toBeCalled();
-  });
 });
 
 describe('Migration from localstorage', () => {

packages/auth-client/src/index.ts

@@ -343,7 +343,10 @@ export class AuthClient {
     }
   }
 
-  private _handleSuccess(message: InternetIdentityAuthResponseSuccess, onSuccess?: () => void) {
+  private async _handleSuccess(
+    message: InternetIdentityAuthResponseSuccess,
+    onSuccess?: () => void,
+  ) {
     const delegations = message.delegations.map(signedDelegation => {
       return {
         delegation: new Delegation(
@@ -380,9 +383,16 @@ export class AuthClient {
         });
       }
     }
-    onSuccess?.();
     this._removeEventListener();
     delete this._idpWindow;
+
+    if (this._chain) {
+      await this._storage.set(KEY_STORAGE_DELEGATION, JSON.stringify(this._chain.toJSON()));
+    }
+
+    // onSuccess should be the last thing to do to avoid consumers
+    // interfering by navigating or refreshing the page
+    onSuccess?.();
   }
 
   public getIdentity(): Identity {
@@ -514,14 +524,7 @@ export class AuthClient {
         case 'authorize-client-success':
           // Create the delegation chain and store it.
           try {
-            this._handleSuccess(message, options?.onSuccess);
-
-            // Setting the storage is moved out of _handleSuccess to make
-            // it a sync function. Having _handleSuccess as an async function
-            // messes up the jest tests for some reason.
-            if (this._chain) {
-              await this._storage.set(KEY_STORAGE_DELEGATION, JSON.stringify(this._chain.toJSON()));
-            }
+            await this._handleSuccess(message, options?.onSuccess);
           } catch (err) {
             this._handleFailure((err as Error).message, options?.onError);
           }