-
Notifications
You must be signed in to change notification settings - Fork 280
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
Conversation
🦋 Changeset detectedLatest commit: 3483068 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
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 |
There was a problem hiding this 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.
3cba27b
to
ea72bc2
Compare
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
5c86472
to
7c61adf
Compare
There was a problem hiding this 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.
if (isOrganizationId(organizationIdOrSlug!)) { | ||
newSession.lastActiveOrganizationId = organizationIdOrSlug || null; | ||
} else { | ||
const matchingOrganization = newSession.user.organizationMemberships.find( |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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:
javascript/packages/react/src/isomorphicClerk.ts
Lines 602 to 608 in 427fcde
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.
There was a problem hiding this comment.
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!
Co-authored-by: Izaak Lauer <[email protected]>
…when setting active org
2dd0d19
to
3aae4eb
Compare
packages/clerk-js/src/core/clerk.ts
Outdated
newSession.lastActiveOrganizationId = organizationId || null; | ||
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id; | ||
|
||
if (isOrganizationId(organizationIdOrSlug!)) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
Co-authored-by: Izaak Lauer <[email protected]>
Co-authored-by: Izaak Lauer <[email protected]>
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_*
).Fixes ORGS-17
Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change