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

Revamp clerk-sdk-node build #612

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Conversation

nikosdouvlis
Copy link
Member

@nikosdouvlis nikosdouvlis commented Dec 11, 2022

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/clerk-js
  • @clerk/clerk-react
  • @clerk/nextjs
  • @clerk/remix
  • @clerk/types
  • @clerk/themes
  • @clerk/localizations
  • @clerk/clerk-expo
  • @clerk/backend
  • @clerk/backend-core
  • @clerk/clerk-sdk-node
  • @clerk/edge
  • @clerk/shared
  • build/tooling/chore

Description

  • npm test runs as expected.
  • npm run build runs as expected.

This PR introduces changes to the build process of the clerk/clerk-sdk-node package. Specifically,

  • Transpile and bundle the package using tsup instead of tsc
  • Correctly export the /esm and /cjs paths while retaining their default exports
  • Introduce the exports field in package.json in preparation for clerk/clerk-sdk-node@5

Supporting all possible host app build configurations (module vs commonjs, pure JS vs TS, and the various moduleResolution strategies that can be used by TS) without introducing breaking changes to the exports of this package is impossible right now.

This PR correctly supports the APIs we've exported so far, with slight modifications that should improve compatibility with the latest node and TS versions.

Currently, the following formats are supported. Any other config combinations the host app package.json / tsconfig files should be invalid, except for the single scenario described in the Caveats section which is a valid config but cannot be supported by clerk-sdk-node currently.

1. Host application configured as a module, or Clerk is used within a module file

This scenario includes one of the following cases:

  • Clerk is used within a .mjs file, or
  • Clerk is used within a .js file and host app's package.json includes "type": "module"
import { users } from '@clerk/clerk-sdk-node'
import Clerk from '@clerk/clerk-sdk-node/esm/instance'

(async () => {
    const clerk = new Clerk({ apiKey: 'test_HoVeVf9Y5cEYZcMXcLDJaljNFUOlCvyNy6'})
    console.log("instance",(await clerk.users.getUserList()).length);
    console.log("direct",(await users.getUserList()).length);
})()

2. Host application configured as a commonjs package, or Clerk is used within a commonjs file

This scenario includes one of the following cases:

  • Clerk is used within a .cjs file, or
  • Clerk is used within a .js file and host app's package.json includes "type": "commonjs" or it does not explicitly specify a type
const { users } = require('@clerk/clerk-sdk-node');
const pkg = require("@clerk/clerk-sdk-node/cjs/instance");
const Clerk = pkg.default;

(async () => {
    const clerk = new Clerk({ apiKey: 'test_HoVeVf9Y5cEYZcMXcLDJaljNFUOlCvyNy6'})
    console.log("instance",(await clerk.users.getUserList()).length);
    console.log("direct",(await users.getUserList()).length);
})()

3. Host application is configured as a module and is using TS

  • package.json includes "type": "module"
  • tsconfig.json sets "module": "ESNext" (or other module strategies)
  • tsconfig.json sets "moduleResolution": "Node"

With the above settings, the generated js file will be a module file. The user can import from /esm and use Clerk directly:

import Clerk from '@clerk/clerk-sdk-node/esm/instance';
const clerk = new Clerk({ apiKey: 'test_HoVeVf9Y5cEYZcMXcLDJaljNFUOlCvyNy6'})

Although unusual, importing a commonjs module is also supported in this case because the generated file is a module. The user can import Clerk from /cjs but they'll need to also use the .default prop:

import Clerk from '@clerk/clerk-sdk-node/cjs/instance';
const clerk = new Clerk.default({ apiKey: 'test_HoVeVf9Y5cEYZcMXcLDJaljNFUOlCvyNy6'})

4. Host application is configured as a commonjs package and is using TS

  • package.json includes "type": "commonjs"
  • tsconfig.json sets "module": "Commonjs" (or other commonjs strategies)
  • tsconfig.json sets "moduleResolution": "Node"

With the above settings, the generated js file will be a commonjs file. The user can import from /cjs and use Clerk directly:

import Clerk from '@clerk/clerk-sdk-node/cjs/instance';
const clerk = new Clerk({ apiKey: 'test_HoVeVf9Y5cEYZcMXcLDJaljNFUOlCvyNy6'})

Caveats

If the host package uses TS and sets "moduleResolution": "NodeNext" or "moduleResolution": "Node16" in tsconfig.json, the types for the default import will not be correctly inferred, so TS will throw an error during compilation even though the runtime value is correct. If these two new module resolution strategies are used, TS will respect the exports field in package.json, however the correct types will not always be inferred because our package exports both default and named exports which are handled differently in ESM and CJS modules.

Possible solutions for the above:

  1. Export 2 builds (cjs, esm) and 2 different sets of types (cjs, esm). Rename the ESM d.ts to d.mts manually so they are always handled as modules.
  2. Drop support for CJS completely
  3. Stop using default exports and stick to using explicitly named exports always.

Plan for v5

v5 will be released once we finish refactoring the package to use the new backend package. I'd suggest we also refactor our exports according to option 3 at the same time, because it is a breaking change.

What we need to do:

  1. Replace default with named exports
  2. Add the following exports field:
  "exports": {
    ".": {
      "types": "./dist/types/index.d.ts",
      "require": "./dist/index.js",
      "import": "./dist/index.mjs",
      "default": "./dist/index.js"
    },
    "./instance": {
      "types": "./dist/types/instance.d.ts",
      "require": "./dist/instance.js",
      "import": "./dist/instance.mjs",
      "default": "./dist/instance.js"
    }
  },

What will change:

  • The /cjs and /esm paths will no longer be needed
  • A single / and /instnace path would handle both esm and cjs formats automatically through the exports field
  • Complete TS support even with the newest module resolution strategies through the exports field

Let me hear your thoughts, happy to discuss further :)

@nikosdouvlis nikosdouvlis force-pushed the js-66-fix-clerk-sdk-node-esm-export_ branch from d2685e3 to 3084825 Compare December 13, 2022 02:49
@nikosdouvlis nikosdouvlis changed the title Js 66 fix clerk sdk node esm export Revamp clerk-sdk-node build Dec 13, 2022
@nikosdouvlis nikosdouvlis force-pushed the js-66-fix-clerk-sdk-node-esm-export_ branch from 3084825 to 0491215 Compare December 13, 2022 03:47
@nikosdouvlis nikosdouvlis merged commit 7758a9c into main Dec 13, 2022
@nikosdouvlis nikosdouvlis deleted the js-66-fix-clerk-sdk-node-esm-export_ branch December 13, 2022 09:33
@clerk clerk deleted a comment from linear bot Dec 13, 2022
onSuccess: 'tsc',
clean: true,
// minify: isProd,
minify: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ why is minify: false ?


import { name, version } from './package.json';

console.log({ name, version });
Copy link
Contributor

Choose a reason for hiding this comment

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

✂️

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants