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

[CJS3] client single instance #599

Merged
merged 4 commits into from
Jan 28, 2021
Merged

Conversation

mahboubii
Copy link
Contributor

@mahboubii mahboubii commented Jan 27, 2021

Unfortunately Lots of TS boilerplates due to generics.

Potential Future improvements: connectUser can return the resolved promise for the second call instead of throwing an error if the user id in the token remains the same

@github-actions
Copy link
Contributor

Size Change: +2.11 kB (0%)

Total Size: 216 kB

Filename Size Change
dist/browser.es.js 46.4 kB +515 B (1%)
dist/browser.full-bundle.min.js 29.7 kB +53 B (0%)
dist/browser.js 47 kB +514 B (1%)
dist/index.es.js 46.4 kB +514 B (1%)
dist/index.js 47 kB +515 B (1%)

compressed-size-action


expect(client1).to.equal(client2);
expect(client2.key).to.eql('key2');
});
Copy link
Contributor

@vishalnarkhede vishalnarkhede Jan 27, 2021

Choose a reason for hiding this comment

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

Can we also add following scenario to unit-test?

const client1 = StreamChat.getInstance('key2');
await client1.setUser({ id: 'vishal });
const client2 = StreamChat.getInstance('key3'); // or key2
await client2.setUser({ id: 'vishal });

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently if you call setUser twice, it throws following error:

'Use client.disconnect() before trying to connect as a different user. connectUser was called twice.'

But I think we should allow it if current user is same as id provided to setUser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed a test d2fcfc3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vishalnarkhede yea I actually put a comment in the PR description about this which is out of the scope of this PR. This is still an integration mistake but we can make it easier for users by not throwing error

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahaa ... sorry missed it!!

Can we add it though ... seems like a simple check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea totally will open another PR on top of this one

@mahboubii mahboubii merged commit c9a1668 into master Jan 28, 2021
@delete-merged-branch delete-merged-branch bot deleted the CJS3-client-single-instnace branch January 28, 2021 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants