chore(astro,shared): Move functions that can be reused by different SDKs #3681

wobsoriano · 2024-07-09T19:46:16Z

Description

This PR tackles ECO-4 and aims to share functions that are currently being duplicated by official and community SDKs.

The ff. functions in the PR are present in the React SDK:

loadClerkJsScript - Duplicated by the ff. community SDKs:

Our work-in-progress Clerk SDK approach and requirements requires Clerk to be hotloaded and having this function shared is a no brainer.

deriveState - While different frameworks have different approaches to SSR, this function is also duplicated across SDKs (Vue, SvelteKit, Astro) and is used to set initial value of a user (data coming from server) even when Clerk is not yet loaded in the client.

All the shared functions above will initially be used by the Astro integration and adopted by different SDKs later.

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

chore(shared): Add deriveState to published files chore(shared): Add deriveState to published files

changeset-bot · 2024-07-09T19:46:20Z

🦋 Changeset detected

Latest commit: 1235e8d

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

This PR includes changesets to release 15 packages

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

packages/shared/src/loadClerkJsScript.ts

LekoArts · 2024-07-10T06:07:42Z

packages/shared/src/deriveState.ts

+
+const deriveFromSsrInitialState = (initialState: InitialState) => {
+  const userId = initialState.userId;
+  const user = initialState.user as UserResource;


Why do we need to typecast some properties in this function?

Good question. Thanks for pointing this out! It was adapted from the React SDK, and I maintained the original structure (including typecasts).

My guess is we want to narrow the types if InitialState is provided? And also some of them are typed as strings (i.e. orgRole, orgPermissions) and typecasting it just for semantics / future-proofihng? Maybe @panteliselef can help me answer this question 😄

I asked because then I think initialState should be typed correctly in the first place 😊

Maybe spend a little bit of time checking if we can fix this on a holistic level, otherwise the type-casts will be fine for now (if other things break)

packages/shared/src/loadClerkJsScript.ts

test(astro): Test script loader version test(astro): Add main script loader test

…ared

wobsoriano · 2024-07-10T19:58:20Z

packages/shared/src/loadClerkJsScript.ts

This function is extracted directly from the React SDK. The only difference is that the versionSelector is removed, which may not be needed for most cases in other SDKs (?) and uses a compile-time env variable (PACKAGE_VERSION).

I'm thinking of putting it back and it moving to shared as well, but remove the compile-time variable. What do you think @LekoArts @panteliselef?

The loadClerkJsScript function needs to behave exactly like the the React SDK one, including the versionSelector functionality. Otherwise things like our snapshot command won't work since the SDK will load the incorrect version of clerk-js on the client.

With some changes it'll also ensure that the correct major version of clerk-js is loaded for the major version of the SDK. With the current hardcoded 5 version you could run into the problem of mismatched versions.

If versionSelector is moved to the shared package we'll need to change the packageVersion = PACKAGE_VERSION fallback in the args and most likely need to change it to JS_PACKAGE_VERSION. Test this!
Reason: clerk-react currently has the same major version as clerk-js, so the logic is sound here. But shared is separate from that, so we should probably replace it to that

Thanks for taking some time to answer this @LekoArts! This makes sense to me.

I have a question for this one - is there gonna be a time that clerk-js and clerk-react will have different versions (other than snapshots)?

Given the fact that versionSelector will be framework specific i think we need to come up with a better way of setting it. I believe each framework should be able to specific its own logic.

@wobsoriano It could happen, for example let's say we wanna a new major for clerk-react because we are switching to react19 and using some of the new features. In that scenario we wouldn't have to cut a new major for clerk-js.

wobsoriano · 2024-07-10T23:10:53Z

packages/astro/src/server/clerk-middleware.ts

-            if (__HOTLOAD__) {
-              controller.enqueue(hotloadScript);
-            }
-


We are no longer attaching the hotload script to the Response since we're using the loadClerkJsScript shared function in client instead.

If for some reason the current implementation is performant than this one, we can adjust to 3-4 commits back where I only used the script builders to clean up some repeating logic but still build the hotload script on the server

We definitely prefer the current implementation because it will result in faster loading times.

You can still use loadClerkJsScript in the client but we will expect to read the existing script.

packages/astro/src/client/index.ts

LekoArts · 2024-07-11T07:18:49Z

packages/astro/src/client/index.ts

@@ -14,7 +17,10 @@ export const createClerkInstance = runOnce(createClerkInstanceInternal);
 export async function createClerkInstanceInternal(options?: AstroClerkIntegrationParams) {
  let clerkJSInstance = window.Clerk;
  if (!clerkJSInstance) {
-    await waitForClerkScript();
+    await loadClerkJsScript({
+      clerkJSVersion: HARDCODED_LATEST_CLERK_JS_VERSION,


We'll need to remove this, I'll explain it in https://github.com/clerk/javascript/pull/3681/files#r1672868119

LekoArts · 2024-07-11T07:20:08Z

packages/astro/src/client/index.ts

-    await waitForClerkScript();
+    await loadClerkJsScript({
+      clerkJSVersion: HARDCODED_LATEST_CLERK_JS_VERSION,
+      ...(options as any),


The as any is a code smell, ideally the options has the correct type, could be a footgun in the future

LekoArts · 2024-07-11T07:21:36Z

packages/astro/src/internal/merge-env-vars-with-params.ts

-    proxyUrl: paramProxy || import.meta.env.PUBLIC_ASTRO_APP_CLERK_PROXY_URL,
-    domain: paramDomain || import.meta.env.PUBLIC_ASTRO_APP_CLERK_DOMAIN,
-    publishableKey: paramPublishableKey || import.meta.env.PUBLIC_ASTRO_APP_CLERK_PUBLISHABLE_KEY || '',
+    // eslint-disable-next-line turbo/no-undeclared-env-vars


Nit: Use block comments (https://eslint.org/docs/latest/use/configure/rules#using-configuration-comments-1) because then it's more concise

LekoArts · 2024-07-11T07:24:36Z

packages/shared/src/deriveState.ts

+
+const deriveFromSsrInitialState = (initialState: InitialState) => {
+  const userId = initialState.userId;
+  const user = initialState.user as UserResource;


I asked because then I think initialState should be typed correctly in the first place 😊

Maybe spend a little bit of time checking if we can fix this on a holistic level, otherwise the type-casts will be fine for now (if other things break)

LekoArts · 2024-07-11T07:31:28Z

packages/shared/src/loadClerkJsScript.ts

The loadClerkJsScript function needs to behave exactly like the the React SDK one, including the versionSelector functionality. Otherwise things like our snapshot command won't work since the SDK will load the incorrect version of clerk-js on the client.

With some changes it'll also ensure that the correct major version of clerk-js is loaded for the major version of the SDK. With the current hardcoded 5 version you could run into the problem of mismatched versions.

If versionSelector is moved to the shared package we'll need to change the packageVersion = PACKAGE_VERSION fallback in the args and most likely need to change it to JS_PACKAGE_VERSION. Test this!
Reason: clerk-react currently has the same major version as clerk-js, so the logic is sound here. But shared is separate from that, so we should probably replace it to that

packages/astro/src/internal/merge-env-vars-with-params.ts

Co-authored-by: Lennart <[email protected]>

packages/shared/src/loadClerkJsScript.ts

packages/shared/src/versionSelector.ts

wobsoriano · 2024-07-11T16:55:31Z

packages/shared/src/versionSelector.ts

As @LekoArts suggested here, the loadClerkJsScript should behave the same way like the React SDK (with the use of versionSelector) because commands like snapshot won't work since the SDK will load an incorrect version of clerk-js.

The default packageVersion prop is now set to the clerk-js version (JS_PACKAGE_VERSION) instead of the clerk-react version (PACKAGE_VERSION).

With this, all SDKs should use the same clerk-js version, unless clerkJsVersion is provided. Thanks!

wobsoriano · 2024-07-15T16:02:10Z

Closing this but it will be tackled in the future at a more relaxing pace.

wobsoriano added 3 commits July 9, 2024 11:53

chore(shared): Add deriveState function

b1bfa60

chore(shared): Add deriveState to published files chore(shared): Add deriveState to published files

chore(astro): Use shared deriveState function

fcae45e

test(shared): Test deriveState with and without initial state

e60b635

wobsoriano added 2 commits July 9, 2024 12:49

test(shared): Handle undefined initialState

a05b2c8

chore(shared): Add and export Clerk script hotload function

542da2d

github-advanced-security bot found potential problems Jul 9, 2024

View reviewed changes

packages/shared/src/loadClerkJsScript.ts Dismissed Show dismissed Hide dismissed

test(astro): clean up

696b221

LekoArts reviewed Jul 10, 2024

View reviewed changes

test(astro): add Clerk script loader tests

401cc89

test(astro): Test script loader version test(astro): Add main script loader test

wobsoriano force-pushed the rob/eco-4-move-or-use-utilities-from-clerkshared branch from edc52b1 to 401cc89 Compare July 10, 2024 19:34

Merge branch 'main' into rob/eco-4-move-or-use-utilities-from-clerksh…

2e4ecc8

…ared

wobsoriano commented Jul 10, 2024

View reviewed changes

wobsoriano added 4 commits July 10, 2024 14:13

chore(astro): Use shared script url builder when hotloading script

69fa644

chore(astro): Use shared data attribute builder for hotloading

a40a55d

chore(astro): Use shared clerk-js hotload script

f159c5e

chore(astro): Clean up type ignores

6af9ea5

wobsoriano commented Jul 10, 2024

View reviewed changes

wobsoriano requested review from LekoArts and panteliselef July 10, 2024 23:13

chore(astro,shared): Add changeset

df670e1

wobsoriano marked this pull request as ready for review July 11, 2024 02:45

LekoArts requested changes Jul 11, 2024

View reviewed changes

LekoArts reviewed Jul 11, 2024

View reviewed changes

packages/astro/src/internal/merge-env-vars-with-params.ts Show resolved Hide resolved

wobsoriano and others added 2 commits July 11, 2024 07:20

chore(astro): Refine script import path

1cd7611

Co-authored-by: Lennart <[email protected]>

chore(shared): Add and export version selector from react package

1235e8d

github-advanced-security bot found potential problems Jul 11, 2024

View reviewed changes

packages/shared/src/loadClerkJsScript.ts Dismissed Show dismissed Hide dismissed

packages/shared/src/versionSelector.ts Dismissed Show dismissed Hide dismissed

wobsoriano commented Jul 11, 2024

View reviewed changes

wobsoriano marked this pull request as draft July 11, 2024 20:57

wobsoriano closed this Jul 15, 2024

wobsoriano mentioned this pull request Aug 1, 2024

chore(astro,clerk-react,shared,nextjs): Migrate functions that can be reused across JavaScript SDKs #3849

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(astro,shared): Move functions that can be reused by different SDKs #3681

chore(astro,shared): Move functions that can be reused by different SDKs #3681

wobsoriano commented Jul 9, 2024 •

edited

Loading

changeset-bot bot commented Jul 9, 2024 •

edited

Loading

LekoArts Jul 10, 2024

wobsoriano Jul 10, 2024 •

edited

Loading

LekoArts Jul 11, 2024

wobsoriano Jul 10, 2024 •

edited

Loading

LekoArts Jul 11, 2024

wobsoriano Jul 11, 2024

panteliselef Jul 11, 2024

wobsoriano Jul 10, 2024 •

edited

Loading

panteliselef Jul 11, 2024

panteliselef Jul 11, 2024

LekoArts Jul 11, 2024

LekoArts Jul 11, 2024

LekoArts Jul 11, 2024

LekoArts Jul 11, 2024

LekoArts Jul 11, 2024

wobsoriano Jul 11, 2024 •

edited

Loading

wobsoriano commented Jul 15, 2024

chore(astro,shared): Move functions that can be reused by different SDKs #3681

chore(astro,shared): Move functions that can be reused by different SDKs #3681

Conversation

wobsoriano commented Jul 9, 2024 • edited Loading

Description

Checklist

Type of change

changeset-bot bot commented Jul 9, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

wobsoriano Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wobsoriano Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wobsoriano Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wobsoriano Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

wobsoriano commented Jul 15, 2024

wobsoriano commented Jul 9, 2024 •

edited

Loading

changeset-bot bot commented Jul 9, 2024 •

edited

Loading

wobsoriano Jul 10, 2024 •

edited

Loading

wobsoriano Jul 10, 2024 •

edited

Loading

wobsoriano Jul 10, 2024 •

edited

Loading

wobsoriano Jul 11, 2024 •

edited

Loading