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

feat(clerk-js): Sign Out from multiple tabs at once #2094

Merged

Conversation

octoper
Copy link
Member

@octoper octoper commented Nov 9, 2023

Description

This PR enables the BrodcastChannel functionallity, that is going to allow us to signout from multiple tabs at once.

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

@octoper octoper requested a review from a team as a code owner November 9, 2023 11:16
Copy link

changeset-bot bot commented Nov 9, 2023

🦋 Changeset detected

Latest commit: 3391990

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@clerk/clerk-js Minor
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@octoper octoper force-pushed the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch from 2f0d78b to 1601f5d Compare November 9, 2023 11:17
@octoper octoper self-assigned this Nov 9, 2023
Copy link
Contributor

@dimkl dimkl left a comment

Choose a reason for hiding this comment

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

I think this is a good candidate for adding a test to our suite.

Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

Welcome back LocalStorageBroadcastChannel :)

@@ -1530,7 +1531,7 @@ export default class Clerk implements ClerkInterface {

this.#broadcastChannel?.addEventListener('message', ({ data }) => {
if (data.type === 'signout') {
void this.handleUnauthenticated({ broadcast: false });
void this.handleUnauthenticated({ broadcast: true });
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's necessary to have broadcast to default to enable post the sign-out message to the BroadcastChannel, but it's not necessary to fill the this. handleUnauthenticated as broadcast prop is always true.

@nikosdouvlis
Copy link
Member

@octoper good job on this! I've approved as I don't want to block, but this is the perfect case for an e2e test. Let's write one before merging.

@octoper
Copy link
Member Author

octoper commented Nov 9, 2023

@nikosdouvlis Already on it's way!

@octoper octoper force-pushed the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch from 1601f5d to e334c0a Compare November 9, 2023 15:00
@octoper octoper force-pushed the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch from da6936f to 4294999 Compare November 10, 2023 09:57
@octoper octoper force-pushed the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch from ec2e590 to d017be1 Compare November 10, 2023 11:58
});
});

expect(await mainTab.page.evaluate('!window.Clerk.user')).toBe(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

🙃 let's add a await m.po.expect.toBeSignedOut(); test helper and use it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The helper exists but there is a problem with it, the page.waitFucntion that is underneath mainTab.po.expect.toBeSignedOut() helper is getting a timeout at this point, I'm not exactly sure why is that, but it's probably has something to do with with the new tab we are opening.

Copy link
Contributor

Choose a reason for hiding this comment

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

let's add a comment about why we cannot use the helper and fix this later.

integration/tests/sign-out-smoke.test.ts Show resolved Hide resolved
@octoper octoper force-pushed the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch from e883e80 to 93278c7 Compare November 10, 2023 18:46
@octoper octoper requested a review from dimkl November 13, 2023 16:21
@octoper octoper force-pushed the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch from 93278c7 to 3391990 Compare November 13, 2023 16:31
@octoper octoper added this pull request to the merge queue Nov 13, 2023
Merged via the queue into main with commit 08dd88c Nov 13, 2023
6 checks passed
@octoper octoper deleted the vaggelis/sdk-767-re-introduce-broadcastchannel-in-clerk-js branch November 13, 2023 17:44
await m.po.expect.toBeSignedOut();
});

expect(await mainTab.page.evaluate('!window.Clerk.user')).toBe(false);
Copy link
Member

Choose a reason for hiding this comment

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

❓ shouldn't this be toBe(true)? If we're trying to replicate toBeSignedOut().

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right! Just opened a PR for that #2125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants