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
6 changes: 6 additions & 0 deletions .changeset/wicked-seahorses-juggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@clerk/clerk-js": patch
"@clerk/types": patch
---

Introduce ability to set an active organization by slug
50 changes: 44 additions & 6 deletions packages/clerk-js/src/core/__tests__/clerk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -335,18 +335,56 @@ describe('Clerk singleton', () => {
return Promise.resolve();
});

await sut.setActive({ organization: { id: 'org-id' } as Organization, beforeEmit: beforeEmitMock });
await sut.setActive({ organization: { id: 'org_id' } as Organization, beforeEmit: beforeEmitMock });

await waitFor(() => {
expect(executionOrder).toEqual(['session.touch', 'set cookie', 'before emit']);
expect(mockSession.touch).toHaveBeenCalled();
expect(mockSession.getToken).toHaveBeenCalled();
expect((mockSession as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org-id');
expect((mockSession as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org_id');
expect(beforeEmitMock).toBeCalledWith(mockSession);
expect(sut.session).toMatchObject(mockSession);
});
});

it('sets active organization by slug', async () => {
const mockSession2 = {
id: '1',
status: 'active',
user: {
organizationMemberships: [
{
id: 'orgmem_id',
organization: {
id: 'org_id',
slug: 'some-org-slug',
},
},
],
},
touch: jest.fn(),
getToken: jest.fn(),
};
mockClientFetch.mockReturnValue(Promise.resolve({ activeSessions: [mockSession2] }));
const sut = new Clerk(productionPublishableKey);
await sut.load();

mockSession2.touch.mockImplementationOnce(() => {
sut.session = mockSession2 as any;
return Promise.resolve();
});
mockSession2.getToken.mockImplementation(() => 'mocked-token');

await sut.setActive({ organization: 'some-org-slug' });

await waitFor(() => {
expect(mockSession2.touch).toHaveBeenCalled();
expect(mockSession2.getToken).toHaveBeenCalled();
expect((mockSession2 as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org_id');
expect(sut.session).toMatchObject(mockSession2);
});
});

mockNativeRuntime(() => {
it('calls session.touch in a non-standard browser', async () => {
mockClientFetch.mockReturnValue(Promise.resolve({ activeSessions: [mockSession] }));
Expand All @@ -365,11 +403,11 @@ describe('Clerk singleton', () => {
return Promise.resolve();
});

await sut.setActive({ organization: { id: 'org-id' } as Organization, beforeEmit: beforeEmitMock });
await sut.setActive({ organization: { id: 'org_id' } as Organization, beforeEmit: beforeEmitMock });

expect(executionOrder).toEqual(['session.touch', 'before emit']);
expect(mockSession.touch).toHaveBeenCalled();
expect((mockSession as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org-id');
expect((mockSession as any as ActiveSessionResource)?.lastActiveOrganizationId).toEqual('org_id');
expect(mockSession.getToken).toBeCalled();
expect(beforeEmitMock).toBeCalledWith(mockSession);
expect(sut.session).toMatchObject(mockSession);
Expand Down Expand Up @@ -1892,12 +1930,12 @@ describe('Clerk singleton', () => {
BaseResource._fetch = jest.fn().mockResolvedValue({});
const sut = new Clerk(developmentPublishableKey);

await sut.getOrganization('some-org-id');
await sut.getOrganization('org_id');

// @ts-expect-error - Mocking a protected method
expect(BaseResource._fetch).toHaveBeenCalledWith({
method: 'GET',
path: '/organizations/some-org-id',
path: '/organizations/org_id',
});
});
});
Expand Down
14 changes: 12 additions & 2 deletions packages/clerk-js/src/core/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ import {
inBrowser,
isDevAccountPortalOrigin,
isError,
isOrganizationId,
isRedirectForFAPIInitiatedFlow,
noOrganizationExists,
noUserExists,
Expand Down Expand Up @@ -707,9 +708,18 @@ export class Clerk implements ClerkInterface {
// However, if the `organization` parameter is not given (i.e. `undefined`), we want
// to keep the organization id that the session had.
const shouldSwitchOrganization = organization !== undefined;

if (newSession && shouldSwitchOrganization) {
const organizationId = typeof organization === 'string' ? organization : organization?.id;
newSession.lastActiveOrganizationId = organizationId || null;
const organizationIdOrSlug = typeof organization === 'string' ? organization : organization?.id;

if (isOrganizationId(organizationIdOrSlug)) {
newSession.lastActiveOrganizationId = organizationIdOrSlug || null;
izaaklauer marked this conversation as resolved.
Show resolved Hide resolved
} 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!

mem => mem.organization.slug === organizationIdOrSlug,
);
newSession.lastActiveOrganizationId = matchingOrganization?.organization.id || null;
}
}

// If this.session exists, then signOut was triggered by the current tab
Expand Down
20 changes: 20 additions & 0 deletions packages/clerk-js/src/utils/__tests__/organization.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { isOrganizationId } from '../organization';

describe('isOrganizationId(string)', () => {
it('should return true for strings starting with `org_`', () => {
expect(isOrganizationId('org_123')).toBe(true);
expect(isOrganizationId('org_abc')).toBe(true);
});

it('should return false for strings not starting with `org_`', () => {
expect(isOrganizationId('user_123')).toBe(false);
expect(isOrganizationId('123org_')).toBe(false);
expect(isOrganizationId('ORG_123')).toBe(false);
});

it('should handle falsy values', () => {
expect(isOrganizationId(undefined)).toBe(false);
expect(isOrganizationId(null)).toBe(false);
expect(isOrganizationId('')).toBe(false);
});
});
1 change: 1 addition & 0 deletions packages/clerk-js/src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,4 @@ export * from './queryStateParams';
export * from './normalizeRoutingOptions';
export * from './image';
export * from './completeSignUpFlow';
export * from './organization';
8 changes: 8 additions & 0 deletions packages/clerk-js/src/utils/organization.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
/**
* Checks and assumes a string is an organization ID if it starts with 'org_', specifically for
* disambiguating with slugs. `_` is a disallowed character in slug names, so slugs cannot
* start with `org_`.
*/
export function isOrganizationId(id: string | null | undefined): boolean {
return typeof id === 'string' && id.startsWith('org_');
}
2 changes: 1 addition & 1 deletion packages/types/src/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,7 @@ export type SetActiveParams = {
session?: ActiveSessionResource | string | null;

/**
* The organization resource or organization id (string version) to be set as active in the current session.
* The organization resource or organization ID/slug (string version) to be set as active in the current session.
* If `null`, the currently active organization is removed as active.
*/
organization?: OrganizationResource | string | null;
Expand Down
Loading