Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: should reattach channel logic #1608

Merged
merged 2 commits into from
Feb 5, 2024
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
12 changes: 6 additions & 6 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -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",
},
},
{
Expand Down
2 changes: 1 addition & 1 deletion src/common/lib/client/realtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
21 changes: 14 additions & 7 deletions src/common/lib/client/realtimechannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -1052,4 +1054,9 @@ class RealtimeChannel extends Channel {
}
}

function omitAgent(channelParams?: API.Types.ChannelParams) {
const { agent: _, ...paramsWithoutAgent } = channelParams || {};
return paramsWithoutAgent;
}

export default RealtimeChannel;
14 changes: 14 additions & 0 deletions test/realtime/channel.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
});
});
});
Loading