Merge pull request #1608 from ably/fix-should-reattach-logic

fix: should reattach channel logic
ably · Feb 5, 2024 · 552f4a3 · 552f4a3
2 parents f463647 + 6b5d083
commit 552f4a3
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 14 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -31,12 +31,12 @@ module.exports = {
     {
       files: ["**/*.{ts,tsx}"],
       rules: {
-	"comma-dangle": ["error", "only-multiline"],
-	"@typescript-eslint/no-unused-vars": ["error"],
-
-	// TypeScript already enforces these rules better than any eslint setup can
-	"no-undef": "off",
-	"no-dupe-class-members": "off",
+        "comma-dangle": ["error", "only-multiline"],
+        "@typescript-eslint/no-unused-vars": ["error", { "varsIgnorePattern": "^_" }],
+        // TypeScript already enforces these rules better than any eslint setup can
+        "no-undef": "off",
+        "no-dupe-class-members": "off",
+        "no-unused-vars": "off",
       },
     },
     {

diff --git a/src/common/lib/client/realtime.ts b/src/common/lib/client/realtime.ts
@@ -149,7 +149,7 @@ class Channels extends EventEmitter {
     if (!channel) {
       channel = this.all[name] = new RealtimeChannel(this.realtime, name, channelOptions);
     } else if (channelOptions) {
-      if (channel._shouldReattachToSetOptions(channelOptions)) {
+      if (channel._shouldReattachToSetOptions(channelOptions, channel.channelOptions)) {
         throw new ErrorInfo(
           'Channels.get() cannot be used to set channel options that would cause the channel to reattach. Please, use RealtimeChannel.setOptions() instead.',
           40000,

diff --git a/src/common/lib/client/realtimechannel.ts b/src/common/lib/client/realtimechannel.ts
@@ -135,6 +135,7 @@ class RealtimeChannel extends Channel {
   }
 
   setOptions(options?: API.Types.ChannelOptions, callback?: ErrCallback): void | Promise<void> {
+    const previousChannelOptions = this.channelOptions;
     if (!callback) {
       if (this.rest.options.promises) {
         return Utils.promisify(this, 'setOptions', arguments);
@@ -154,7 +155,7 @@ class RealtimeChannel extends Channel {
     }
     Channel.prototype.setOptions.call(this, options);
     if (this._decodingContext) this._decodingContext.channelOptions = this.channelOptions;
-    if (this._shouldReattachToSetOptions(options)) {
+    if (this._shouldReattachToSetOptions(options, previousChannelOptions)) {
       /* This does not just do _attach(true, null, callback) because that would put us
        * into the 'attaching' state until we receive the new attached, which is
        * conceptually incorrect: we are still attached, we just have a pending request to
@@ -184,24 +185,25 @@ class RealtimeChannel extends Channel {
     }
   }
 
-  _shouldReattachToSetOptions(options?: API.Types.ChannelOptions) {
+  _shouldReattachToSetOptions(options: API.Types.ChannelOptions | undefined, prevOptions: API.Types.ChannelOptions) {
     if (!(this.state === 'attached' || this.state === 'attaching')) {
       return false;
     }
     if (options?.params) {
       // Don't check against the `agent` param - it isn't returned in the ATTACHED message
-      const requestedParams = Object.assign({}, options.params);
-      delete requestedParams.agent;
-      if (Object.keys(requestedParams).length !== 0 && !this.params) {
+      const requestedParams = omitAgent(options.params);
+      const existingParams = omitAgent(prevOptions.params);
+
+      if (Object.keys(requestedParams).length !== Object.keys(existingParams).length) {
         return true;
       }
 
-      if (this.params && !Utils.shallowEquals(this.params, requestedParams)) {
+      if (!Utils.shallowEquals(existingParams, requestedParams)) {
         return true;
       }
     }
     if (options?.modes) {
-      if (!this.modes || !Utils.arrEquals(options.modes, this.modes)) {
+      if (!prevOptions.modes || !Utils.arrEquals(options.modes, prevOptions.modes)) {
         return true;
       }
     }
@@ -1052,4 +1054,9 @@ class RealtimeChannel extends Channel {
   }
 }
 
+function omitAgent(channelParams?: API.Types.ChannelParams) {
+  const { agent: _, ...paramsWithoutAgent } = channelParams || {};
+  return paramsWithoutAgent;
+}
+
 export default RealtimeChannel;
diff --git a/test/realtime/channel.test.js b/test/realtime/channel.test.js
@@ -1684,5 +1684,19 @@ define(['ably', 'shared_helper', 'async', 'chai'], function (Ably, helper, async
         });
       });
     });
+
+    it('should not throw exception then run RealtimeChannels.get() with same options', function (done) {
+      const realtime = helper.AblyRealtime();
+      const channel = realtime.channels.get('channel-with-options', { modes: ['PRESENCE'] });
+      channel.attach();
+      channel.whenState('attaching', function () {
+        try {
+          realtime.channels.get('channel-with-options', { modes: ['PRESENCE'] });
+          closeAndFinish(done, realtime);
+        } catch (err) {
+          closeAndFinish(done, realtime, err);
+        }
+      });
+    });
   });
 });