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.

Sign up for GitHub

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

chore(clerk-js): Reorganize cookies code and fix TokenUpdate event #3362

Merged

dimkl merged 9 commits into main from chore/reorganize-cookies

May 15, 2024

Member

dimkl commented May 10, 2024 •

edited

Description

Re-organize cookie codebase into a central place, fix TokenUpdate event to be triggered on sign-out and drop duplicate event on refreshing token.

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

dimkl requested a review from nikosdouvlis

May 10, 2024 13:59

dimkl self-assigned this

changeset-bot bot commented May 10, 2024 •

edited

🦋 Changeset detected

Latest commit: f731bfe

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

This PR includes changesets to release 13 packages

Name	Type
@clerk/clerk-js	Patch
@clerk/shared	Patch
@clerk/chrome-extension	Patch
@clerk/clerk-expo	Patch
@clerk/backend	Patch
@clerk/elements	Patch
@clerk/express	Patch
@clerk/fastify	Patch
@clerk/nextjs	Patch
@clerk/clerk-react	Patch
@clerk/remix	Patch
@clerk/clerk-sdk-node	Patch
@clerk/testing	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

clerk-cookie added the clerk-js label

dimkl force-pushed the chore/reorganize-cookies branch from c1f4859 to 62a7170 Compare

May 10, 2024 14:00

dimkl commented

View reviewed changes

packages/shared/src/cookie.ts Show resolved Hide resolved

dimkl mentioned this pull request

feat(backend,clerk-js,shared): Introduce suffixed / un-suffixed cookies #3274

Open

11 tasks

BRKalow reviewed

View reviewed changes

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated

+// TODO: make SessionCookieService singleton since it handles updating cookies using a poller
+// and we need to avoid updating them concurrently.
+export class SessionCookieService {

Member

BRKalow May 10, 2024

It's hard to see the diff of this file as it was moved...can you outline the changes made here briefly?

Member Author

dimkl May 13, 2024

We probably need to rename the service to AuthCookieService. My intention is to create an interface that can be used both for Browser / Native apps and use 2 different implementations. The SessionCookieService will be used for Browser.

Changes:

move code cookies related with auth to this service
introduce auth related helper methods that use auth cookies to avoid exposing cookies handling outside of this module
apply changes due to refactoring cookies exposed interface

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated Show resolved Hide resolved

packages/clerk-js/src/core/auth/cookies/clientUat.ts Show resolved Hide resolved

packages/clerk-js/src/core/clerk.ts Show resolved Hide resolved

dimkl commented

View reviewed changes

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated

- if (!inBrowser()) {
- return;
- }

Member Author

dimkl May 13, 2024

ℹ️ since this class will only be initialized in standard browsers, there is no need to guard for non browsers.

dimkl commented

View reviewed changes

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated

 if (!this.clerk.session) {

 return;

 }

 try {

 this.updateSessionCookie(await this.clerk.session?.getToken());

 await this.clerk.session.getToken();

Member Author

dimkl May 13, 2024

ℹ️ Since the this.clerk.session.getToken() triggers the events.TokenUpdate event which updates the session cookie there is no need to invoke this.updateSessionCookie.

dimkl commented

View reviewed changes

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated

- return setSessionCookie(rawToken);
- }
- return removeSessionCookie();
+ private updateSessionCookie(token: string | null) {

Member Author

dimkl May 13, 2024

ℹ️ simplified the function signature (dropped the TokenResource) since it's being used internally this is not a breaking change.

dimkl commented

View reviewed changes

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated Show resolved Hide resolved

BRKalow approved these changes

View reviewed changes

Member

BRKalow left a comment

This looks good to me! Approving so you can merge tomorrow, but would like to see some inline comments added based on my initial review.

Nice cleanup!

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated Show resolved Hide resolved

dimkl force-pushed the chore/reorganize-cookies branch from 62a7170 to 0e9fb89 Compare

May 14, 2024 11:39

nikosdouvlis reviewed

View reviewed changes

packages/shared/src/cookie.ts Show resolved Hide resolved

packages/clerk-js/src/core/auth/cookies/devBrowser.ts Show resolved Hide resolved

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated Show resolved Hide resolved

packages/clerk-js/src/core/auth/SessionCookieService.ts Outdated Show resolved Hide resolved

dimkl force-pushed the chore/reorganize-cookies branch from 4c7a342 to d9d1b7f Compare

May 14, 2024 12:03

Member Author

dimkl commented May 14, 2024

@BRKalow Could you take a look at the following commits (PR comments are addressed):

Member Author

dimkl commented May 14, 2024

@nikosdouvlis We removed the return statement because we didn't use it and it would make the CookieHandler method signatures used in clerk-js return values the same as the shared/cookie cookieHandler which will allow us in the future to merge them if less changes.
We can drop that change if you think it's gonna cause issues or if it does not give us enough value to change it.
ref: #3362 (comment)

BRKalow reviewed

View reviewed changes

packages/clerk-js/src/core/auth/AuthCookieService.ts Outdated Show resolved Hide resolved

dimkl added 8 commits

May 15, 2024 10:40


fix(shared): Change createCookieHandler#.set to return void

6d3a8f7


chore(clerk-js): Centralize cookie code into single module

fd9586a


chore(clerk-js): Refactor SessionCookieService and auth/cookies handl…

6adb791

…ers for consistency

There is also a fix for duplicate TokenUpdate event
and for cleaning the token in sign-out flow.


chore(clerk-js): Refactor Clerk.#environment modifier to protected to…

b17a2c2

… use type assert


chore(repo): Add changeset

c9d68ed


chore(clerk-js): Rename SessionCookieService to AuthCookieService for…

fc997d8

… clarity


chore(clerk-js): Add JSDoc for auth services and cookies

a7150fa


chore(clerk-js): Rename urlWithAuth to decorateUrlWithDevBrowserToken…

ed019b0

… method of auth service

nikosdouvlis approved these changes

View reviewed changes

packages/clerk-js/src/core/auth/cookies/clientUat.ts Show resolved Hide resolved


fix(clerk-js): Fix isSignedOut() to use both clientUat and clerk.user

f731bfe

This change was applied to keep the existing behaviour:
- clerk.user is used when clerk is loaded
- clientUat cookie is used when clerk is NOT loaded

dimkl force-pushed the chore/reorganize-cookies branch from d9d1b7f to f731bfe Compare

May 15, 2024 09:51

dimkl merged commit ec84d51 into main

10 checks passed

dimkl deleted the chore/reorganize-cookies branch

May 15, 2024 10:02

clerk-cookie mentioned this pull request

Version Packages #3381

Merged

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