Fix for member based channel #586

vishalnarkhede · 2021-01-15T13:48:55Z

Submit a pull request

CLA

I have signed the Stream CLA (required).
The code changes follow best practices
Code changes are tested (add some information if not applicable)

Description of the pull request

Existing logic:
We maintain a list of active channels on client - client.activeChannel.

So every-time, user requests a channel using client.channel('messaging', 'random_channel_id'), we first check if the cid already exist in client.activeChannels
|--> if yes, then return the channel available on client.activeChannels
|--> if not, then create new instance of Channel class and return that.

This way we ensure that user doesn't end up creating multiple copies of same channel. And this is important, because when client receives ws event, it delegates it to all channels in activeChannel map. So if your channel isn't part of activeChannels, it won't receive any of the events.

Issue

This logic only worked for id based channels. If user requests a member based channel (e.g., client.channel('messaging', { members: ['vishal', 'amin'] })), then we skipped that check, because the logic for generating id from members list is on backend side, and its not a good idea to expose it (checked with @thesyncim ) on frontend. So we couldn't really check if the channel already exists in client.activeChannels

Fix

We are adding this missing check for member based channels.

For member based channel, we don't get cid until channel is watched/queried. So until we get the key from backend, we are going to store it in activeChannels with the key ${channelType}:!members-${members.sort().join(',')}. E.g. messaging:!members-amin,vishal or messaging:!members-amin,jaap,tom. So that we can ensure uniqueness even if channel is not queries/watched yet.

We assert that a particular channel in activeChannels is same as requested channel by user:

key === ${channelType}:!members-${members.sort().join(',')}

OR

If key starts with ${channelType}:!members- AND
channel has same members as the channel which is requested.

…hal/thread_participants_type

github-actions · 2021-01-15T13:51:01Z

Size Change: +4.03 kB (1%)

Total Size: 214 kB

Filename	Size	Change
`dist/browser.es.js`	45.9 kB	+944 B (2%)
`dist/browser.full-bundle.min.js`	29.6 kB	+227 B (0%)
`dist/browser.js`	46.5 kB	+954 B (2%)
`dist/index.es.js`	45.9 kB	+945 B (2%)
`dist/index.js`	46.5 kB	+957 B (2%)

_{compressed-size-action}

…m/stream-chat-js into vishal/thread_participants_type

test/integration/channels.js

mahboubii · 2021-01-15T15:14:44Z

test/integration/channels.js

+		const clientVish = await getTestClientForUser(userIdVish);
+		const channelVish_copy1 = clientVish.channel('messaging', {
+			members: ['amin', 'vishal'],
+		});
+		await channelVish_copy1.watch();
+		const channelVish_copy2 = clientVish.channel('messaging', {
+			members: ['vishal', 'amin'],
+		});


kinda an edge case to this fix which itself is an edge case but if you call channel.watch or channel.create after instantiating the second channel you still end up with a duplicate channel in the state. (this can actually happen if different parts of the app instantiate channels at the same time)

const channelVish_copy1 = clientVish.channel('messaging', { members: ['amin', 'vishal'] }); const channelVish_copy2 = clientVish.channel('messaging', { members: ['amin', 'vishal'] }); // channelVish_copy1 !== channelVish_copy2 await channelVish_copy1.watch(); await channelVish_copy2.watch(); // channelVish_copy1 !== channelVish_copy2

hmm probably we should early return from watch() function if its already being watched

async watch(options) { if (this.initialized) { return; } ... // rest of the logic }

yea it's a potential fix but there could be changes in the option param which cause other issues. To cover this we need to make some changes to the new code you implemented to also check for channels that don't have their cid yet but have the same member list

Makes sense!! Have updated the PR.

So when channel doesn't have cid yet, I am storing it in activeChannels as following:

{ "messaging:amin,vishal": uniqueChannelAminVishal, "messaging:k4j2n3k4j2n3kn42k3": channelWithId }

When cid gets set on channel, I replace messaging:amin,vishal with actual cid generated from backend.

{ "messaging:!members:kajshdkjsakjsdkjas": uniqueChannelAminVishal, "messaging:k4j2n3k4j2n3kn42k3": channelWithId }

Also updated tests to reflect this case :)

…m/stream-chat-js into vishal/thread_participants_type

ferhatelmas

LGTM

ferhatelmas · 2021-01-18T12:23:36Z

src/client.ts

+   * Its a submethod for `client.channel()` method, used to create unique conversation or
+   * channel based on member list instead of id.
+   *
+   * If the channel already exist in `activeChannels` list, then we simply return it, since that


ferhatelmas · 2021-01-18T12:23:41Z

src/client.ts

+   * If the channel already exist in `activeChannels` list, then we simply return it, since that
+   * means the same channel was already requested or created.
+   *
+   * Otherwise we create new instance of Channel class and return it.


ferhatelmas · 2021-01-18T12:23:46Z

src/client.ts

+  }
+
+  /**
+   * Its a submethod for `client.channel()` method, used to create unique conversation or


ferhatelmas · 2021-01-18T12:24:36Z

src/client.ts

+    // channel could exist in `activeChannels` list with either one of the following two keys:
+    // 1. cid - Which gets set on channel only after calling channel.query or channel.watch or channel.create
+    // 2. Sorted membersStr - E.g., "messaging:amin,vishal" OR "messaging:amin,jaap,tom"
+    //                        This gets set when you create a channel, but haven't queried yet. After query,


ferhatelmas · 2021-01-18T12:25:35Z

src/client.ts

+  };
+
+  /**
+   * Its a submethod for `client.channel()` method, used to channel given the id of channel.


submethod doesn't sound intuitive, a helper maybe

also, let's add a @private to these two functions

ferhatelmas · 2021-01-18T12:25:42Z

src/client.ts

+  /**
+   * Its a submethod for `client.channel()` method, used to channel given the id of channel.
+   *
+   * If the channel already exist in `activeChannels` list, then we simply return it, since that


…m/stream-chat-js into vishal/thread_participants_type

…t with backend

…ants_type

…m/stream-chat-js into vishal/thread_participants_type

vishalnarkhede · 2021-01-19T19:35:39Z

@mahboubii added !members- to membersStr !!

mahboubii · 2021-01-20T08:18:09Z

src/channel.ts

+      const membersStr = Object.keys(this.state.members)?.sort().join(',');
+
+      if (`${this.type}:!members-${membersStr}` in this.getClient().activeChannels) {
+        delete this.getClient().activeChannels[membersStr];


should we delete the first instance from the activeChannels?

What do you mean? Its a map, so there will be only one instance.

But just noticed that we are not deleting the correct key from activeChannels. It should be something like:

delete this.getClient().activeChannels[`${this.type}:!members-${membersStr}`];

Gonna add to test as well :)

vishalnarkhede · 2021-01-20T15:04:51Z

Moved tests to unit-test :)

vishalnarkhede added 2 commits January 4, 2021 13:26

Merge branch 'master' of github.com:GetStream/stream-chat-js into vis…

32cc933

…hal/thread_participants_type

Ensure unique member based channel on client

5f16699

vishalnarkhede requested review from mahboubii, ferhatelmas and thesyncim January 15, 2021 14:01

vishalnarkhede added 3 commits January 15, 2021 15:04

Merge branch 'master' into vishal/thread_participants_type

38bfcb9

Adding test

8852374

Merge branch 'vishal/thread_participants_type' of github.com:GetStrea…

30841cd

…m/stream-chat-js into vishal/thread_participants_type

ferhatelmas reviewed Jan 15, 2021

View reviewed changes

test/integration/channels.js Outdated Show resolved Hide resolved

Updated the test

506d6be

mahboubii reviewed Jan 15, 2021

View reviewed changes

Amin Mahboubi and others added 6 commits January 15, 2021 16:59

Merge branch 'master' into vishal/thread_participants_type

40bc02c

Cover the edge cases of channel creation

de2ce01

Merge branch 'vishal/thread_participants_type' of github.com:GetStrea…

3491ee4

…m/stream-chat-js into vishal/thread_participants_type

Adding channelType to temporary cid of unique conversation

39d3db1

Fixing members related error

877bb57

Update test title

3abf6c9

mahboubii approved these changes Jan 18, 2021

View reviewed changes

ferhatelmas approved these changes Jan 18, 2021

View reviewed changes

ferhatelmas reviewed Jan 18, 2021

View reviewed changes

src/client.ts Outdated

}

/**

* Its a submethod for `client.channel()` method, used to create unique conversation or

Copy link

Contributor

ferhatelmas Jan 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's

ferhatelmas reviewed Jan 18, 2021

View reviewed changes

vishalnarkhede added 4 commits January 18, 2021 15:27

Fixing comments

9065826

Merge branch 'vishal/thread_participants_type' of github.com:GetStrea…

5b6d6b9

…m/stream-chat-js into vishal/thread_participants_type

Fixing comment grammer

f8e9e72

Merge branch 'master' into vishal/thread_participants_type

763c4ab

vishalnarkhede added 5 commits January 18, 2021 23:21

Merge branch 'master' into vishal/thread_participants_type

3ff1be9

Updated temporary cid for member based channels, to keep it consisten…

fcba1a6

…t with backend

Merge github.com:GetStream/stream-chat-js into vishal/thread_particip…

ba0d5c3

…ants_type

Merge branch 'vishal/thread_participants_type' of github.com:GetStrea…

21c2664

…m/stream-chat-js into vishal/thread_participants_type

Fixing code comment

f4cb2aa

mahboubii reviewed Jan 20, 2021

View reviewed changes

vishalnarkhede added 3 commits January 20, 2021 13:20

Fixing tempCid cleanup from activeChannels

f24efb7

Fixing memberStr during channel.query

4ef8f28

Moving test to unit-test

9c7abc8

vishalnarkhede merged commit 1d9daa6 into master Jan 20, 2021

delete-merged-branch bot deleted the vishal/thread_participants_type branch January 20, 2021 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for member based channel #586

Fix for member based channel #586

vishalnarkhede commented Jan 15, 2021 •

edited

Loading

github-actions bot commented Jan 15, 2021 •

edited

Loading

mahboubii Jan 15, 2021 •

edited

Loading

vishalnarkhede Jan 15, 2021 •

edited

Loading

mahboubii Jan 15, 2021

vishalnarkhede Jan 18, 2021

ferhatelmas left a comment

ferhatelmas Jan 18, 2021

ferhatelmas Jan 18, 2021

ferhatelmas Jan 18, 2021

ferhatelmas Jan 18, 2021

ferhatelmas Jan 18, 2021

ferhatelmas Jan 18, 2021 •

edited

Loading

mahboubii Jan 18, 2021

vishalnarkhede Jan 18, 2021

ferhatelmas Jan 18, 2021

vishalnarkhede commented Jan 19, 2021

mahboubii Jan 20, 2021

vishalnarkhede Jan 20, 2021

vishalnarkhede Jan 20, 2021

vishalnarkhede commented Jan 20, 2021

Fix for member based channel #586

Fix for member based channel #586

Conversation

vishalnarkhede commented Jan 15, 2021 • edited Loading

Submit a pull request

CLA

Description of the pull request

github-actions bot commented Jan 15, 2021 • edited Loading

mahboubii Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

vishalnarkhede Jan 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferhatelmas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferhatelmas Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnarkhede commented Jan 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnarkhede commented Jan 20, 2021

vishalnarkhede commented Jan 15, 2021 •

edited

Loading

github-actions bot commented Jan 15, 2021 •

edited

Loading

mahboubii Jan 15, 2021 •

edited

Loading

vishalnarkhede Jan 15, 2021 •

edited

Loading

ferhatelmas Jan 18, 2021 •

edited

Loading