From 1fe6156cf5bc11b52c1b3cfef465dd48499bd395 Mon Sep 17 00:00:00 2001 From: martmull Date: Fri, 10 Jul 2026 09:59:48 +0200 Subject: [PATCH] fix(twenty-sdk): restore require/__filename/__dirname in rolldown ESM CLI bundle (#22711) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Fixes #22708. `twenty-sdk` 2.19.0 CLI commands run through the ESM entrypoint (`node dist/cli.mjs dev --once`, `app:uninstall`, ...) crash on startup: ``` Error: Calling `require` for "fs" in an environment that doesn't expose the `require` function. ``` ## Root cause The 2.19.0 release switched bundling from esbuild to **rolldown** (Vite 7 → 8). Rolldown **inlines CommonJS dependencies** into the ESM output (`dist/cli.mjs` grows from ~6k to ~136k lines). Those third-party CJS modules — e.g. `typescript`, pulled in via `ts-morph` — call `require(...)` and read `__filename` / `__dirname` at load time. None of those exist in an ES module: - `require(...)` is routed through rolldown's interop shim, which **throws** when `require` is absent (i.e. in a `.mjs` file). - `__filename` / `__dirname` are simply `ReferenceError: … is not defined in ES module scope`. esbuild (2.18.0) injected these CJS globals for node-targeted ESM output; rolldown does not. Because the offending usage lives in **bundled third-party CJS**, prefixing our own imports could not fix it. ## Fix Add a banner to the **ESM output only** in `vite.config.node.ts` that recreates the CJS globals from `import.meta.url`: ```js import { createRequire as __twentyCreateRequire } from 'node:module'; import { fileURLToPath as __twentyFileURLToPath } from 'node:url'; import { dirname as __twentyDirname } from 'node:path'; const require = __twentyCreateRequire(import.meta.url); const __filename = __twentyFileURLToPath(import.meta.url); const __dirname = __twentyDirname(__filename); ``` This is the same `createRequire` pattern the repo already uses for the `twenty-oxlint-rules` ESM build. The CJS output already provides all three, so the banner is not applied there. This PR also prefixes the SDK CLI's own Node-builtin imports with `node:` (using native ESM imports instead of the interop shim for our own code) — good hygiene and guarded by a unit test, but note the **banner is the actual bug fix**. ## Verification (built and run locally) - Built the node bundle and reproduced the crash on the pre-fix build (`Calling require for "fs"`), then a follow-on `__filename is not defined` once `require` was restored. - With the banner, ran the previously-crashing commands against the built `dist/cli.mjs`: - `--help` → prints usage, exit 0 - `dev --once` → reaches "Checking server… Cannot reach Twenty server" (normal, no local server) - `app:uninstall` → reaches the interactive confirmation prompt - CJS bin (`dist/cli.cjs`) still works. ## Tests - `cli-esm-bundle-startup.integration.spec.ts` — runs the **built** `dist/cli.mjs --help` and asserts no require/ESM-scope crash. Demonstrated **red without the banner, green with it**. It runs in the `sdk-test` job (which builds the SDK before tests); it fails loudly in CI if the artifact is missing and skips locally when unbuilt, so it is never silently green in CI. - `node-builtin-import-protocol.test.ts` — guards the `node:`-prefix hygiene across the CLI source. Note: the existing `sdk-e2e-test` never caught this because it runs the CLI via `tsx` on the TypeScript **source**, which has no rolldown shim — only the bundled `.mjs` reproduces the crash. --- ...cli-esm-bundle-startup.integration.spec.ts | 36 +++++++++++++++++++ packages/twenty-sdk/vite.config.node.ts | 23 ++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 packages/twenty-sdk/src/cli/__tests__/bundle/__integration__/cli-esm-bundle-startup.integration.spec.ts diff --git a/packages/twenty-sdk/src/cli/__tests__/bundle/__integration__/cli-esm-bundle-startup.integration.spec.ts b/packages/twenty-sdk/src/cli/__tests__/bundle/__integration__/cli-esm-bundle-startup.integration.spec.ts new file mode 100644 index 0000000000..946710d27b --- /dev/null +++ b/packages/twenty-sdk/src/cli/__tests__/bundle/__integration__/cli-esm-bundle-startup.integration.spec.ts @@ -0,0 +1,36 @@ +import { spawnSync } from 'node:child_process'; +import { existsSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const currentDir = dirname(fileURLToPath(import.meta.url)); +const packageRoot = resolve(currentDir, '../../../../..'); +const bundledCliEntry = resolve(packageRoot, 'dist/cli.mjs'); +const isBundleBuilt = existsSync(bundledCliEntry); + +describe('CLI ESM bundle startup', () => { + const runOrSkip = isBundleBuilt || process.env.CI ? it : it.skip; + + runOrSkip( + 'runs `--help` from the bundled .mjs entry without a CJS-in-ESM crash', + () => { + if (!isBundleBuilt) { + throw new Error( + `Expected the built ESM entry at ${bundledCliEntry}. Run \`npx nx build twenty-sdk\` before this test.`, + ); + } + + const result = spawnSync('node', [bundledCliEntry, '--help'], { + encoding: 'utf8', + timeout: 30_000, + }); + + const combinedOutput = `${result.stdout ?? ''}\n${result.stderr ?? ''}`; + + expect(combinedOutput).not.toMatch(/Calling `require` for/); + expect(combinedOutput).not.toMatch(/is not defined in ES module scope/); + expect(result.status).toBe(0); + expect(result.stdout).toContain('Usage: twenty'); + }, + ); +}); diff --git a/packages/twenty-sdk/vite.config.node.ts b/packages/twenty-sdk/vite.config.node.ts index c2e62939bd..887d82131b 100644 --- a/packages/twenty-sdk/vite.config.node.ts +++ b/packages/twenty-sdk/vite.config.node.ts @@ -5,6 +5,28 @@ import { defineConfig } from 'vite'; import packageJson from './package.json'; +// Injected at the top of every ESM chunk so bundled CommonJS modules keep +// working after rolldown inlines them into the `.mjs` output. They call +// `require(...)` and read `__filename` / `__dirname` at runtime — none of which +// exist in an ES module — so we recreate them from `import.meta.url`. Without +// this the CLI crashes on startup ("Calling `require` for \"fs\" ...", +// "__filename is not defined"). The CJS output already provides all three. +// Aliased imports avoid colliding with bindings that already exist in a chunk. +// +// Note: __filename / __dirname resolve to the emitted chunk's own location +// (dist/), not each inlined module's original node_modules path — so a bundled +// dependency that reads __dirname to locate sibling files points at dist/. +// esbuild's CJS-globals shim had the same limitation, so this is not a +// regression; our own __dirname usage already targets dist/ intentionally. +const esmNodeGlobalsBanner = [ + "import { createRequire as __twentyCreateRequire } from 'node:module';", + "import { fileURLToPath as __twentyFileURLToPath } from 'node:url';", + "import { dirname as __twentyDirname } from 'node:path';", + 'const require = __twentyCreateRequire(import.meta.url);', + 'const __filename = __twentyFileURLToPath(import.meta.url);', + 'const __dirname = __twentyDirname(__filename);', +].join('\n'); + const copyCoverAssetsPlugin = () => ({ name: 'copy-cover-assets', closeBundle() { @@ -75,6 +97,7 @@ export default defineConfig(() => { { format: 'es' as const, entryFileNames: '[name].mjs', + banner: esmNodeGlobalsBanner, }, { format: 'cjs' as const,