feat(clerk-js): Add ability to set active organization by slug#3825
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 |
izaaklauer
left a comment
There was a problem hiding this comment.
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
izaaklauer
left a comment
There was a problem hiding this comment.
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.
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.
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.
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
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.
This is a good client piggybacking intro. Thanks izaak!
Co-authored-by: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com>
…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.
I'd probably opt for removing the ! and add falsy handling to isOrganizationId + add tests for passing in undefined
Co-authored-by: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com>
Co-authored-by: Izaak Lauer <8404559+izaaklauer@users.noreply.github.com>
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 testruns as expected.npm run buildruns as expected.Type of change