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): Add ability to set active organization by slug #3825

Merged
merged 7 commits into from
Jul 31, 2024

Conversation

wobsoriano
Copy link
Member

@wobsoriano wobsoriano commented Jul 25, 2024

Description

This PR introduces ability to set an active organization by slug and updates existing org tests to use a proper Org ID format (org_*).

setActive({ organization: 'some-cool-slug' })

Fixes ORGS-17

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:

Copy link

changeset-bot bot commented Jul 25, 2024

🦋 Changeset detected

Latest commit: 3483068

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

This PR includes changesets to release 18 packages
Name Type
@clerk/clerk-js Patch
@clerk/types Patch
@clerk/astro Patch
@clerk/chrome-extension Patch
@clerk/clerk-expo Patch
@clerk/backend Patch
@clerk/elements Patch
@clerk/express Patch
@clerk/fastify Patch
@clerk/localizations Patch
@clerk/nextjs Patch
@clerk/clerk-react Patch
@clerk/remix Patch
@clerk/clerk-sdk-node Patch
@clerk/shared Patch
@clerk/tanstack-start Patch
@clerk/testing Patch
@clerk/themes 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

Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

Looking good I think!

I wonder if there are any tests we could add for passing a slug into setActive.

packages/clerk-js/src/core/clerk.ts Outdated Show resolved Hide resolved
packages/clerk-js/src/core/clerk.ts Show resolved Hide resolved
@wobsoriano wobsoriano changed the title Add ability to set active organization by slug feat(core): Add ability to set active organization by slug Jul 26, 2024
@wobsoriano wobsoriano changed the title feat(core): Add ability to set active organization by slug feat(clerk-js): Add ability to set active organization by slug Jul 26, 2024
@wobsoriano wobsoriano force-pushed the rob/brand-218-accept-slug-from-setactive branch from 3cba27b to ea72bc2 Compare July 26, 2024 16:12
chore(clerk-js): Remove deprecated test text

chore(clerk-js): Simplify organization slug or id checking

chore(clerk-js): Clean up org id condition

chore(clerk-js): Use updated or current session when checking for org id

test(clerk-js): Update org IDs in tests to use correct format

chore(clerk-js): Use organization id instead of org membership id

test(clerk-js): Add organization id checker test

test(clerk-js): Test setting active organization by slug

test(clerk-js): Move new test to last of setActive
@wobsoriano wobsoriano force-pushed the rob/brand-218-accept-slug-from-setactive branch from 5c86472 to 7c61adf Compare July 26, 2024 16:23
@wobsoriano wobsoriano marked this pull request as ready for review July 26, 2024 17:12
Copy link
Contributor

@izaaklauer izaaklauer left a comment

Choose a reason for hiding this comment

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

Overall this looks good to me!

🔧 As a part of this PR, I think we should also update the docs for SetActiveParams to clarify that it can take a slug.

packages/clerk-js/src/utils/organization.ts Outdated Show resolved Hide resolved
packages/clerk-js/src/utils/organization.ts Outdated Show resolved Hide resolved
if (isOrganizationId(organizationIdOrSlug!)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
} else {
const matchingOrganization = newSession.user.organizationMemberships.find(
Copy link
Contributor

@izaaklauer izaaklauer Jul 29, 2024

Choose a reason for hiding this comment

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

I have two concerns here, borne of my own experience:

1: 🤔 How confident are we that newSession.user.organizationMemberships will always present and populated in this case? I don't think you need to await anything to get access to setActive and call it, so I could imagine a scenario where we haven't yet completed the backend call to /tokens. But I may be missing something!

2: 🤔 Even if we can depend on newSession being populated, this smells to me like it's being fed via client piggybacking, and i've heard that we'd rather make API calls for the data that we need explicitly rather than rely on client piggybacking. But again, I maybe misunderstanding something!

Copy link
Contributor

@izaaklauer izaaklauer Jul 29, 2024

Choose a reason for hiding this comment

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

I'd be happy to pair and trace to find those answers with you @wobsoriano . And I think it's likely that someone with more experience may know the answer to these off-hand - they strike me as foundational-sdk-philosophy questions 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I happened to chat with @BRKalow , and I got my answers!

1: The magic here is that, when someone calls useClerk, they're getting IsomorphicClerk, who'se implementation of setActive is here:

setActive = ({ session, organization, beforeEmit }: SetActiveParams): Promise<void> => {
if (this.clerkjs) {
return this.clerkjs.setActive({ session, organization, beforeEmit });
} else {
return Promise.reject();
}
};

If clerk hasn't loaded the client by the time the user calls setActive, it'll return return Promise.reject();. And if clerk has loaded, we'll have that session data. So this is safe?

2: This isn't client piggybacking supplying this data. Client piggybacking is us returning the latest client data on api calls other than the explicit call to load the client, but it's the explicit call that we're making when clerk loads.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good client piggybacking intro. Thanks izaak!

@wobsoriano wobsoriano force-pushed the rob/brand-218-accept-slug-from-setactive branch from 2dd0d19 to 3aae4eb Compare July 29, 2024 19:43
newSession.lastActiveOrganizationId = organizationId || null;
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug!)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably opt for removing the ! and add falsy handling to isOrganizationId + add tests for passing in undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@LekoArts LekoArts merged commit 80e6477 into main Jul 31, 2024
18 checks passed
@LekoArts LekoArts deleted the rob/brand-218-accept-slug-from-setactive branch July 31, 2024 07:18
BRKalow pushed a commit that referenced this pull request Aug 1, 2024
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.

4 participants