Repository: parse-community/Parse-SDK-JS
Pull request #1727 (2023-02-03 23:02:12)

fix: `LiveQuerySubscription.unsubscribe` resolves promise before unsubscribing completes
## Pull Request

- Report security issues [confidentially](https://github.com/parse-community/Parse-SDK-JS/security/policy).
- Any contribution is under this [license](https://github.com/parse-community/Parse-SDK-JS/blob/alpha/LICENSE).
- Link this pull request to an [issue](https://github.com/parse-community/Parse-SDK-JS/issues?q=is%3Aissue).

## Issue
<!-- Add the link to the issue that this PR closes. -->

Closes: https://github.com/parse-community/Parse-SDK-JS/issues/1720

## Approach
<!-- Describe the changes in this PR. -->

* Ensure promise resolves
* Remove duplicate test

## Tasks
<!-- Delete tasks that don't apply. -->

- [x] Add tests
- [x] Add changes to documentation (guides, repository pages, code comments)


parse-github-assistant[bot]: 
### Thanks for opening this pull request!

- ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

<!-- parse-issue-bot-meta-tag-id {"requireTemplate":true} -->


codecov[bot]: # [Codecov](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1727?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) Report
Base: **99.89**% // Head: **99.89**% // Increases project coverage by **`+0.00%`** :tada:
> Coverage data is based on head [(`e519851`)](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1727?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) compared to base [(`23b8810`)](https://codecov.io/gh/parse-community/Parse-SDK-JS/commit/23b8810da444a56fca286020de73541d5a0993b4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community).
> Patch coverage: 100.00% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##            alpha    #1727   +/-   ##
=======================================
  Coverage   99.89%   99.89%           
=======================================
  Files          61       61           
  Lines        5984     5990    +6     
  Branches     1372     1373    +1     
=======================================
+ Hits         5978     5984    +6     
  Misses          6        6           
```


| [Impacted Files](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1727?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community) | Coverage Δ | |
|---|---|---|
| [src/LiveQueryClient.js](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1727?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-c3JjL0xpdmVRdWVyeUNsaWVudC5qcw==) | `99.01% <100.00%> (+0.02%)` | :arrow_up: |
| [src/LiveQuerySubscription.js](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1727?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community#diff-c3JjL0xpdmVRdWVyeVN1YnNjcmlwdGlvbi5qcw==) | `100.00% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/parse-community/Parse-SDK-JS/pull/1727?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=parse-community).

mtrezza: Is this a breaking change? If yes, can it be made a non-breaking change?
dplewis: This isn't a breaking change
mtrezza: I'm asking because you [wrote](https://github.com/parse-community/Parse-SDK-JS/issues/1720#issue-1566993870):

> Using a promise would be a breaking change but would be aligned with subscribe
dplewis: I realized that `unsubscribe` already returned a promise. It just ran in the background and still does after this PR.
mtrezza: Got it, thanks for confirming, just wanted to make sure
mtrezza: So is this actually a bugfix then because the promise resolved without the unsub actually having finished? Like:

> fix: `LiveQuerySubscription.unsubscribe` resolves promise before unsubscribing finishes
dplewis: Thats correct

Patch:
diff --git a/src/LiveQueryClient.js b/src/LiveQueryClient.js
index 7e8f4d8..17b4ab4 100644
--- a/src/LiveQueryClient.js
+++ b/src/LiveQueryClient.js
@@ -224,19 +224,20 @@ class LiveQueryClient extends EventEmitter {
    * After calling unsubscribe you'll stop receiving events from the subscription object.
    *
    * @param {object} subscription - subscription you would like to unsubscribe from.
+   * @returns {Promise | undefined}
    */
-  unsubscribe(subscription: Object) {
+  unsubscribe(subscription: Object): ?Promise {
     if (!subscription) {
       return;
     }
-
-    this.subscriptions.delete(subscription.id);
     const unsubscribeRequest = {
       op: OP_TYPES.UNSUBSCRIBE,
       requestId: subscription.id,
     };
-    this.connectPromise.then(() => {
-      this.socket.send(JSON.stringify(unsubscribeRequest));
+    return this.connectPromise.then(() => {
+      return this.socket.send(JSON.stringify(unsubscribeRequest));
+    }).then(() => {
+      return subscription.unsubscribePromise;
     });
   }
 
@@ -400,9 +401,14 @@ class LiveQueryClient extends EventEmitter {
       }
       break;
     }
-    case OP_EVENTS.UNSUBSCRIBED:
-      // We have already deleted subscription in unsubscribe(), do nothing here
+    case OP_EVENTS.UNSUBSCRIBED: {
+      if (subscription) {
+        this.subscriptions.delete(data.requestId);
+        subscription.subscribed = false;
+        subscription.unsubscribePromise.resolve();
+      }
       break;
+    }
     default: {
       // create, update, enter, leave, delete cases
       if (!subscription) {
diff --git a/src/LiveQuerySubscription.js b/src/LiveQuerySubscription.js
index 1c6c638..503a071 100644
--- a/src/LiveQuerySubscription.js
+++ b/src/LiveQuerySubscription.js
@@ -99,6 +99,7 @@ class Subscription extends EventEmitter {
     this.query = query;
     this.sessionToken = sessionToken;
     this.subscribePromise = resolvingPromise();
+    this.unsubscribePromise = resolvingPromise();
     this.subscribed = false;
 
     // adding listener so process does not crash
@@ -115,8 +116,8 @@ class Subscription extends EventEmitter {
     return CoreManager.getLiveQueryController()
       .getDefaultLiveQueryClient()
       .then(liveQueryClient => {
-        liveQueryClient.unsubscribe(this);
         this.emit('close');
+        return liveQueryClient.unsubscribe(this);
       });
   }
 }


Test patch:
diff --git a/src/__tests__/LiveQueryClient-test.js b/src/__tests__/LiveQueryClient-test.js
index ba98ea5..a0dbd76 100644
--- a/src/__tests__/LiveQueryClient-test.js
+++ b/src/__tests__/LiveQueryClient-test.js
@@ -72,34 +72,6 @@ describe('LiveQueryClient', () => {
     liveQueryClient.open();
   });
 
-  it('can unsubscribe', async () => {
-    const liveQueryClient = new LiveQueryClient({
-      applicationId: 'applicationId',
-      serverURL: 'ws://test',
-      javascriptKey: 'javascriptKey',
-      masterKey: 'masterKey',
-      sessionToken: 'sessionToken',
-    });
-    liveQueryClient.socket = {
-      send: jest.fn(),
-    };
-    const subscription = {
-      id: 1,
-    };
-    liveQueryClient.subscriptions.set(1, subscription);
-
-    liveQueryClient.unsubscribe(subscription);
-    liveQueryClient.connectPromise.resolve();
-    expect(liveQueryClient.subscriptions.size).toBe(0);
-    await liveQueryClient.connectPromise;
-    const messageStr = liveQueryClient.socket.send.mock.calls[0][0];
-    const message = JSON.parse(messageStr);
-    expect(message).toEqual({
-      op: 'unsubscribe',
-      requestId: 1,
-    });
-  });
-
   it('can handle open / close states', () => {
     const liveQueryClient = new LiveQueryClient({
       applicationId: 'applicationId',
@@ -284,6 +256,7 @@ describe('LiveQueryClient', () => {
     });
     const subscription = new events.EventEmitter();
     subscription.subscribePromise = resolvingPromise();
+    subscription.unsubscribePromise = resolvingPromise();
 
     liveQueryClient.subscriptions.set(1, subscription);
     const data = {
@@ -295,7 +268,7 @@ describe('LiveQueryClient', () => {
       data: JSON.stringify(data),
     };
     liveQueryClient._handleWebSocketMessage(event);
-    expect(liveQueryClient.subscriptions.size).toBe(1);
+    expect(liveQueryClient.subscriptions.size).toBe(0);
   });
 
   it('can handle WebSocket error response message', async () => {
@@ -871,12 +844,13 @@ describe('LiveQueryClient', () => {
     };
     const subscription = {
       id: 1,
+      unsubscribePromise: resolvingPromise(),
     };
     liveQueryClient.subscriptions.set(1, subscription);
 
     liveQueryClient.unsubscribe(subscription);
     liveQueryClient.connectPromise.resolve();
-    expect(liveQueryClient.subscriptions.size).toBe(0);
+    expect(liveQueryClient.subscriptions.size).toBe(1);
     await liveQueryClient.connectPromise;
     const messageStr = liveQueryClient.socket.send.mock.calls[0][0];
     const message = JSON.parse(messageStr);
@@ -884,6 +858,14 @@ describe('LiveQueryClient', () => {
       op: 'unsubscribe',
       requestId: 1,
     });
+    const event = {
+      data: JSON.stringify({
+        op: 'unsubscribed',
+        requestId: 1,
+      }),
+    };
+    liveQueryClient._handleWebSocketMessage(event);
+    expect(liveQueryClient.subscriptions.size).toBe(0);
   });
 
   it('can unsubscribe without subscription', async () => {
diff --git a/src/__tests__/ParseLiveQuery-test.js b/src/__tests__/ParseLiveQuery-test.js
index b6b55f0..63178f0 100644
--- a/src/__tests__/ParseLiveQuery-test.js
+++ b/src/__tests__/ParseLiveQuery-test.js
@@ -227,7 +227,7 @@ describe('ParseLiveQuery', () => {
     });
   });
 
-  it('should not throw on usubscribe', done => {
+  it('should not throw on usubscribe', () => {
     CoreManager.set('UserController', {
       currentUserAsync() {
         return Promise.resolve({
@@ -240,7 +240,7 @@ describe('ParseLiveQuery', () => {
     const query = new ParseQuery('ObjectType');
     query.equalTo('test', 'value');
     const subscription = new LiveQuerySubscription('0', query, 'token');
-    subscription.unsubscribe().then(done).catch(done.fail);
+    subscription.unsubscribe();
   });
 
   it('can handle LiveQuery open event', async () => {