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.

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

Jump to bottom

chore(clerk-js): Simplify dev browser handling #2345

Merged
merged 1 commit into from
Dec 15, 2023
Merged

chore(clerk-js): Simplify dev browser handling #2345

merged 1 commit into from
Dec 15, 2023

Conversation

SokratisVidros
Copy link
Contributor

Description

  1. Work only with URL-based session syncing dev instances and drop support for cookie-based syncing dev instances.
  2. Align nomenclature across cookie, hearer, and search param names
  3. Drop cookie handler in favor of dedicated cookie handlers, one for each cookie (__clerk_db_jwt, __session, __client_uat)

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:

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

@SokratisVidros SokratisVidros self-assigned this Dec 14, 2023
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Dec 14, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 0917fed

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

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

@SokratisVidros SokratisVidros force-pushed the simplify_dev_browser_handling branch from f8bde01 to 94021cd Compare December 14, 2023 09:58
@SokratisVidros SokratisVidros force-pushed the simplify_dev_browser_handling branch from 94021cd to a954168 Compare December 14, 2023 10:15
nikosdouvlis
nikosdouvlis approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@nikosdouvlis nikosdouvlis left a comment

Choose a reason for hiding this comment

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

We need to apply this changes to AP as well, correct?

packages/shared/src/devBrowser.ts
export const DEV_BROWSER_SSO_JWT_PARAMETER = '__clerk_db_jwt';
export const DEV_BROWSER_JWT_MARKER = '__clerk_db_jwt';
export const DEV_BROWSER_SSO_JWT_KEY = 'clerk-db-jwt';
export const DEV_BROWSER_JWT_KEY = '__clerk_db_jwt';
Copy link
Member

Choose a reason for hiding this comment

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

Finally!

packages/nextjs/src/server/constants.ts Outdated
@@ -2,7 +2,7 @@ import { apiUrlFromPublishableKey } from '@clerk/shared/apiUrlFromPublishableKey
import { isTruthy } from '@clerk/shared/underscore';

export const CLERK_JS_VERSION = process.env.NEXT_PUBLIC_CLERK_JS_VERSION || '';
export const CLERK_JS_URL = process.env.NEXT_PUBLIC_CLERK_JS || '';
export const CLERK_JS_URL = process.env.NEXT_PUBLIC_CLERK_JS_URL || '';
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change as the current variable is called NEXT_PUBLIC_CLERK_JS

@dimkl we need to include this in the changelog/ upgrade guide

packages/clerk-js/src/ui/customizables/parseVariables.ts Outdated
@@ -73,7 +73,7 @@ export const createFontSizeScale = (theme: Theme): Record<keyof typeof fontSizes
return {
xs: (numericValue * 0.6875).toString() + unit,
sm: (numericValue * 0.75).toString() + unit,
md: (numericValue * 0.8125).toString() + unit,
md: (numericValue * 1).toString() + unit,
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be reverted

@dimkl dimkl closed this Dec 15, 2023
@dimkl dimkl reopened this Dec 15, 2023
@SokratisVidros SokratisVidros force-pushed the simplify_dev_browser_handling branch from c16d653 to 3e66133 Compare December 15, 2023 13:43
brkalow
brkalow approved these changes Dec 15, 2023
View reviewed changes
Copy link
Member

@brkalow brkalow left a comment

Choose a reason for hiding this comment

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

Looks good!

packages/clerk-js/src/core/devBrowser.ts Outdated
});

fapiClient.onAfterResponse((_, response) => {
const newDevBrowserJWT = response?.headers?.get('Clerk-Db-Jwt');
Copy link
Member

Choose a reason for hiding this comment

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

(nit) Can we make this a constant that lives with the others?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

packages/clerk-js/src/core/devBrowser.ts
setDevBrowserCookie(existingDevBrowserCookie);
}

// 2. Get the JWT from hash or search parameters when the redirection comes from AP
Copy link
Member

Choose a reason for hiding this comment

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

Hedging considering we're going to drop the hash transport mechanism:

Suggested change
// 2. Get the JWT from hash or search parameters when the redirection comes from AP
// 2. Get the JWT from the URL when the redirection comes from AP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I have a to-do item for this, but I'd like us to see how V4 and V5 will work in AP before the final change. So we will get back to this next week.

packages/clerk-js/src/utils/cookies/session.ts
Comment on lines +15 to +17
export const removeSessionCookie = () => sessionCookie.remove();

export const setSessionCookie = (token: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

I like this new pattern 👍

packages/clerk-js/src/core/devBrowser.ts
fapiClient: FapiClient;
};

export function createDevBrowser({ frontendApi, fapiClient }: CreateDevBrowserOptions): DevBrowser {
Copy link
Member

Choose a reason for hiding this comment

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

❓ why not make this a class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a huge fan of classes 😆

@SokratisVidros
chore(clerk-js): Simplify dev browser handling
0917fed 
1. Work only with URL based session syncing dev instances and drop support for cookie-based syncing dev instances.
2. Align nomenclature across cookie, hearer and search param names
3. Drop cookie handler in favor of dedicated cookie handlers, one for each cookie (__clerk_db_jwt, __session, __client_uat)
@SokratisVidros SokratisVidros force-pushed the simplify_dev_browser_handling branch from 3e66133 to 0917fed Compare December 15, 2023 13:59
@SokratisVidros SokratisVidros merged commit 7e76b41 into main Dec 15, 2023
3 checks passed
@SokratisVidros SokratisVidros deleted the simplify_dev_browser_handling branch December 15, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikosdouvlis nikosdouvlis nikosdouvlis approved these changes

@brkalow brkalow brkalow approved these changes

@tmilewski tmilewski Awaiting requested review from tmilewski

@dimkl dimkl Awaiting requested review from dimkl

Assignees

@SokratisVidros SokratisVidros

Labels
chrome-extension clerk-js nextjs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SokratisVidros @brkalow @nikosdouvlis @dimkl @clerk-cookie