BREAKING(shared): Revamp package to use subpaths by LekoArts · Pull Request #1898 · clerk/javascript

LekoArts · 2023-10-17T08:58:24Z

Description

This is a bigger PR, so buckle up for this description and reviewing the code.

First, I've created two separate PRs that enabled this PR:

fix(shared): Remove testUtils & smaller updates #1868
- Removed testUtils from shared and cleaned up the output a bit already
chore(repo,clerk-js): Update TS to v5 & Update emotion minor #1877
- Updated to TS 5 so that I can use moduleResolution: "Bundler" in this PR

Overview

Now onto what this PR this.

The TL;DR is: This PR enables module subpaths for @clerk/shared. You'll be able to write this:

import { deprecated } from "@clerk/shared/deprecated"
import { OrganizationProvider } from "@clerk/shared/react"

You can read the Why section on why we do this change. This PR fixes #1883

Fixes SDK-509

This PR is deemed a breaking change because we're removing every React-related import from the main import, moving it to the /react subpath.

Other Changes

If you go through the changed files list, you'll see that in most files only the imports were changed, from @clerk/shared to @clerk/shared/<name>. You'll also notice that I didn't do this for every single module, sometimes I left a bigger import { stuff } from "@clerk/shared" in there. My rule of thumb was to have maximum 3-4 separate imports, otherwise it gets a bit too wild. The separate imports are specifically for functions where we care about splitting them out, e.g. react or deprecated.
The next change you'll see often is inside tsconfig.json. Most/all of our config files defined the moduleResolution as node. To quote:

'node10' (previously called 'node') for Node.js versions older than v10, which only support CommonJS require. You probably won’t need to use node10 in modern code.

I also referenced the TSConfig Cheat Sheet a lot. I think in theory I could have changed most things to use Bundler but I want to be cautious in this PR here and only changed it to NodeNext. This allows using module subpaths. When using moduleResolution: "NodeNext" you also need to set module to NodeNext. I used Bundler once as I was forced to use it, see GitHub inline comment.

Sidenote: Our tsconfig.json setup is a mess, but I already created an internal ticket to clean that up
As smaller change I did: The functionality of inClientSide and inBrowser was the same. So I removed inClientSide and replaced its imports with inBrowser
The webpack configuration of clerk-js needed an update, too. Since @clerk/shared will use .mjs file extensions now, webpack needed an update for its extensions config and the ts-loader
Where appropriate, I changed export * from "" to export { stuff } from "" as this is clearer what gets exported
I removed the shared file inside sdk-node as we now fully rely on @clerk/shared

`@clerk/shared` Changes

See GitHub inline comments

Why

Building a bundler is hard, so we shouldn't expect that they can all "automagically" handle the weirdness we live in right now: CJS + ESM, Barrel files, Tree-Shaking of optional parts, etc.

We can help bundlers/Node to decide what to parse, load, analyze, and bundle by defining module subpaths. By using them we can make @clerk/shared completely isomorphic by separating out specific code that is only relevant in certain contexts (e.g. React only in React context).

Another benefit this now gives us is that we can shape the public API and force users/us into the right patterns. Since react isn't exported from the root index file of @clerk/shared, you're forced to use @clerk/shared/react instead. So React is completely isolated from the rest of the package.

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

Packages affected

LekoArts · 2023-10-17T12:07:33Z

+  return {
+    entry: ['./src/**/*.{ts,tsx}', '!./src/**/*.test.{ts,tsx}'],
+    format: ['cjs', 'esm'],
+    bundle: true,


We need to have .mjs file extensions in imports of .mjs files - there's no way around it.

And for that we need to enable bundle, see egoist/tsup#953

There's a papertrail of related issues for that:

Use explicit extensions systematically egoist/tsup#565

[feature] Generate esm-specific declaration files egoist/tsup#760

https://github.com/favware/esbuild-plugin-file-path-extensions

Missing file extension on the import statement evanw/esbuild#622

Have we considered the approach outlined here?

On a separate but related note, .mjs extensions used to break CRA apps and this is the reason we didn't follow the ESM spec completely up until now and also the reason we used legacyOutput here. I will need time to find old support requests for this, but I'm torn if this is something we want to treat as a breaking change or not at this point.

We need to discuss whether a build/bundle change on our side can be considered a breaking change. If the public API of our package has not changed, and the issue is caused by an outdated bundler on the user's side, I think we should consider this a non breaking change.

Have we considered the approach outlined egoist/tsup#953 (comment)?

For that we'd need to add type: "module" to our package.json which could have larger implications. Because the workaround then renames CJS .js files to .cjs because .js will be treated as ESM by default then

What I've seen from e.g. https://github.com/sindresorhus is that when they made packages ESM-only, it was a breaking change. Us adjusting the way how we output ESM here, I wouldn't consider that a breaking change.

Anyways, we're already publishing this as a new major, so it's fine?

brkalow · 2023-10-17T19:46:46Z

+export { createWorkerTimers } from './workerTimers';
+export * from './browser';
+export * from './color';
+export * from './date';
+export * from './deprecated';
+export * from './error';
+export * from './file';
+export { handleValueOrFn } from './handleValueOrFn';
+export { isomorphicAtob } from './isomorphicAtob';
+export * from './keys';
+export { loadScript } from './loadScript';
+export { LocalStorageBroadcastChannel } from './localStorageBroadcastChannel';
+export * from './poller';
+export * from './proxy';
+export * from './underscore';
+export * from './url';


❓ Why are we retaining the index file here if we're shifting everything to use the exported subpaths?

(Also see the first bulletpoint in the the "Other changes" PR description)

With this PR I don't want to completely kill productivity for the sake of diminishing returns on optimizations. The purpose of the PR is to allow subpaths so that we (the engineers) can use them where appropriate.

If I'd remove this index file, some files would have 10+ imports without giving any actual benefit - because all these functions exported from this file here are fine to be not completely separated. They don't use any heavy libraries, they don't assume a completely different environment (like react). So they won't cause any problems if they might be analyzed before being tree-shaken.

Hope that makes sense :)

With the proposed setup, is there any difference between * exports and named exports as far as tree-shaking is concerned?

No, no difference. Important bit for this file is to not export certain stuff to keep it separate

brkalow · 2023-10-17T19:47:35Z

    sourcemap: true,
-    legacyOutput: true,
-    external: ['@testing-library', 'react', 'jest', 'jest-environment-jsdom', 'react-dom'],
+    dts: true,


❓ Any idea how much slower this is than generating the declarations separately?

tsup + tsc : npm run build 2.64s user 0.26s system 195% cpu 1.485 total
tsup : npm run build 4.53s user 0.60s system 184% cpu 3.486 total

So it is slower (egoist/tsup#945)

I'm fine with revisiting this is a follow-up, the relative speed loss is large, the absolute not so much (at least for this package)

Thanks, agreed the absolute difference is reasonable 😃

brkalow · 2023-10-17T19:50:56Z

+    "moduleResolution": "NodeNext",
+    "module": "NodeNext",


❓ why the change here?

See the PR description, section "Other changes"

brkalow · 2023-10-17T19:51:20Z

+    "module": "NodeNext",
+    "moduleResolution": "NodeNext",


❓ Why the change here?

See the PR description, section "Other changes"

LekoArts · 2023-10-18T07:23:13Z

!snapshot

clerk-cookie · 2023-10-18T07:26:12Z

Hey @LekoArts - the snapshot version command generated the following package versions:

Package	Version
@clerk/backend	0.31.2-snapshot.vf9232b3
@clerk/chrome-extension	0.4.9-snapshot.vf9232b3
@clerk/clerk-js	4.62.1-snapshot.vf9232b3
@clerk/clerk-expo	0.19.11-snapshot.vf9232b3
@clerk/fastify	0.6.16-snapshot.vf9232b3
gatsby-plugin-clerk	4.4.17-snapshot.vf9232b3
@clerk/localizations	1.26.6-snapshot.vf9232b3
@clerk/nextjs	4.25.6-snapshot.vf9232b3
@clerk/clerk-react	4.26.6-snapshot.vf9232b3
@clerk/remix	3.0.8-snapshot.vf9232b3
@clerk/clerk-sdk-node	4.12.15-snapshot.vf9232b3
@clerk/shared	1.0.0-snapshot.vf9232b3
@clerk/types	3.56.1-snapshot.vf9232b3

Tip: Use the snippet copy button below to quickly install the required packages.
@clerk/backend

npm i @clerk/backend@0.31.2-snapshot.vf9232b3 --save-exact

@clerk/chrome-extension

npm i @clerk/chrome-extension@0.4.9-snapshot.vf9232b3 --save-exact

@clerk/clerk-js

npm i @clerk/clerk-js@4.62.1-snapshot.vf9232b3 --save-exact

@clerk/clerk-expo

npm i @clerk/clerk-expo@0.19.11-snapshot.vf9232b3 --save-exact

@clerk/fastify

npm i @clerk/fastify@0.6.16-snapshot.vf9232b3 --save-exact

gatsby-plugin-clerk

npm i gatsby-plugin-clerk@4.4.17-snapshot.vf9232b3 --save-exact

@clerk/localizations

npm i @clerk/localizations@1.26.6-snapshot.vf9232b3 --save-exact

@clerk/nextjs

npm i @clerk/nextjs@4.25.6-snapshot.vf9232b3 --save-exact

@clerk/clerk-react

npm i @clerk/clerk-react@4.26.6-snapshot.vf9232b3 --save-exact

@clerk/remix

npm i @clerk/remix@3.0.8-snapshot.vf9232b3 --save-exact

@clerk/clerk-sdk-node

npm i @clerk/clerk-sdk-node@4.12.15-snapshot.vf9232b3 --save-exact

@clerk/shared

npm i @clerk/shared@1.0.0-snapshot.vf9232b3 --save-exact

@clerk/types

npm i @clerk/types@3.56.1-snapshot.vf9232b3 --save-exact

dimkl · 2023-10-18T10:08:49Z

🙃 rename files to packages/shared/src/workerTimers/types.ts and packages/shared/src/workerTimers/worker.ts since the workerTimers exists as folder in path

We rely on the naming here:

https://github.com/clerkinc/javascript/blob/3bf64107e1d0f9fce55163276d157da7849a390f/packages/shared/tsup.config.ts#L50

I didn't want to break anything so I just kept it as-is :)

nikosdouvlis · 2023-10-18T10:45:07Z

  "name": "@clerk/shared",
  "version": "0.24.4",
  "description": "Internal package utils used by the Clerk SDKs",
-  "types": "./dist/types/index.d.ts",


What about users still on previous TS versions? Are there types exported from clerk/shared that could affect our public types? Probably not, asking to make sure.

nikosdouvlis · 2023-10-18T10:49:31Z

-      "types": "./dist/types/index.d.ts",
-      "import": "./dist/esm/index.js",
-      "require": "./dist/cjs/index.js"
+      "import": {


Correct - sharing some extra context:

Older Clerk packages used ^ to reference other Clerk dependencies. We are not using pinned versions but we should be careful and make this a major bump as we don't want to break apps depending on a fixed-version Clerk package (in the user package.json), that in turns depends on clerk/shared@^0

nikosdouvlis · 2023-10-18T11:04:48Z

+export { createWorkerTimers } from './workerTimers';
+export * from './browser';
+export * from './color';
+export * from './date';
+export * from './deprecated';
+export * from './error';
+export * from './file';
+export { handleValueOrFn } from './handleValueOrFn';
+export { isomorphicAtob } from './isomorphicAtob';
+export * from './keys';
+export { loadScript } from './loadScript';
+export { LocalStorageBroadcastChannel } from './localStorageBroadcastChannel';
+export * from './poller';
+export * from './proxy';
+export * from './underscore';
+export * from './url';


With the proposed setup, is there any difference between * exports and named exports as far as tree-shaking is concerned?

nikosdouvlis · 2023-10-18T11:08:46Z

+  return {
+    entry: ['./src/**/*.{ts,tsx}', '!./src/**/*.test.{ts,tsx}'],
+    format: ['cjs', 'esm'],
+    bundle: true,


Have we considered the approach outlined here?

nikosdouvlis · 2023-10-18T11:18:52Z

+  return {
+    entry: ['./src/**/*.{ts,tsx}', '!./src/**/*.test.{ts,tsx}'],
+    format: ['cjs', 'esm'],
+    bundle: true,


On a separate but related note, .mjs extensions used to break CRA apps and this is the reason we didn't follow the ESM spec completely up until now and also the reason we used legacyOutput here. I will need time to find old support requests for this, but I'm torn if this is something we want to treat as a breaking change or not at this point.

We need to discuss whether a build/bundle change on our side can be considered a breaking change. If the public API of our package has not changed, and the issue is caused by an outdated bundler on the user's side, I think we should consider this a non breaking change.

nikosdouvlis · 2023-10-18T11:24:09Z

-    outDir: './dist/cjs',
-  };
-
-  return runAfterLast(['npm run build:declarations', shouldPublish && 'npm run publish:local'])(esm, cjs);


We're losing yalc publishing here but let's revisit it after this is released

My assumption is that we'll move to secco sooner than later, so I removed it 😅

brkalow

🚢

clerk-cookie · 2024-10-19T00:01:52Z

This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

LekoArts added 30 commits October 12, 2023 09:53

chore(shared): Smaller package.json maintenance

234a01a

chore(repo): Remove testUtils from shared pkg

6c209ea

chore(shared): Update package.json more

46c5b55

chore(shared): Jest cleanup

b5409e3

chore(shared): Move noop to root

ca49589

chore(shared): Move browser and remove duplicate inClientSide helper

ccd4282

chore(shared): Move method helpers into "underscore" file

21ec0ef

chore(shared): Move url to root

0357351

chore(shared): Fix jest testRegex

707247a

chore(shared): Tsup updates

3e85daf

chore(shared): Move color into single file

b6850a2

chore(shared): Move proxy to root

5d776fc

chore(shared): Move error stuff into error dir

abb5056

chore(shared): Move date to root

c53ad69

chore(shared): Move keys to root

6fdeaf5

chore(shared): Move deprecated to root

30391c4

chore(shared): Move isomorphicAtob to root

6477433

chore(shared): Move cookie to folder

ea774b6

chore(shared): Move workerTimers

74ddfcb

chore(shared): Move file and remove mimeTypeExtensions

797c495

chore(shared): Move cookie and globs

235e2e0

chore(shared): Tsup ignore test files

c678874

chore(shared): Move rest of files to root

e271a80

chore(shared): Re-organize hooks folder

0aa5f68

chore(shared): Correct tsup output + exports map

54a52a6

chore(shared): Add minimal README

85489c0

fix(backend): Update imports

5506ab8

chore(shared): Put error into one file + add react & react/hooks subpath

7fd38a3

fix(clerk-js): Update imports

feb461d

fix(clerk-expo): Updat imports

3a7b7af

LekoArts commented Oct 17, 2023

View reviewed changes

LekoArts added 3 commits October 17, 2023 14:25

Merge branch 'main' into lekoarts/js-509-revamp-clerkshared-package

24538e5

chore(repo): Update lock file

626b336

chore(shared): Move react/hooks into react

0fbb88f

LekoArts changed the title ~~feat(shared): Revamp package to use subpaths~~ BREAKING(shared): Revamp package to use subpaths Oct 17, 2023

Update wild-carrots-reply.md

7847fcd

SokratisVidros mentioned this pull request Oct 17, 2023

Include clerkTraceId for backend api errors #1894

Merged

24 tasks

brkalow reviewed Oct 17, 2023

View reviewed changes

LekoArts marked this pull request as ready for review October 18, 2023 05:53

LekoArts requested a review from a team as a code owner October 18, 2023 05:53

LekoArts added 2 commits October 18, 2023 07:53

Merge branch 'main' into lekoarts/js-509-revamp-clerkshared-package

65d64d9

fix(shared): Use correct entries for tsup

f9232b3

LekoArts added 2 commits October 18, 2023 10:19

chore(repo): Add changelog entry

e6bf6f8

Merge branch 'main' into lekoarts/js-509-revamp-clerkshared-package

d093817

dimkl approved these changes Oct 18, 2023

View reviewed changes

dimkl reviewed Oct 18, 2023

View reviewed changes

nikosdouvlis approved these changes Oct 18, 2023

View reviewed changes

chore(clerk-js): Add comment

6b4e081

brkalow approved these changes Oct 18, 2023

View reviewed changes

LekoArts added this pull request to the merge queue Oct 19, 2023

Merged via the queue into main with commit aa4cd76 Oct 19, 2023

LekoArts deleted the lekoarts/js-509-revamp-clerkshared-package branch October 19, 2023 06:19

clerk-cookie mentioned this pull request Oct 19, 2023

Version Packages #1915

Merged

LekoArts mentioned this pull request Oct 23, 2023

Cannot find module 'react' #1927

Closed

4 tasks

panteliselef mentioned this pull request Nov 1, 2023

fix(nextjs): Import deprecatedObjectProperty from subpath #1997

Merged

24 tasks

clerk locked as resolved and limited conversation to collaborators Oct 19, 2024

Uh oh!

Conversation

LekoArts commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Overview

Other Changes

@clerk/shared Changes

Why

Checklist

Type of change

Packages affected

Uh oh!

LekoArts Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LekoArts Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LekoArts commented Oct 18, 2023

Uh oh!

clerk-cookie commented Oct 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brkalow left a comment

Choose a reason for hiding this comment

Uh oh!

clerk-cookie commented Oct 19, 2024

Uh oh!

Reviewers

Assignees

Labels

LekoArts commented Oct 17, 2023 •

edited

Loading

`@clerk/shared` Changes

LekoArts Oct 17, 2023 •

edited

Loading

LekoArts Oct 18, 2023 •

edited

Loading