[nest] Support CommonJS compilation for NestJS projects #982
Conversation
🦋 Changeset detectedLatest commit: d5bbcb0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 packages
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 |
|
@boomyao is attempting to deploy a commit to the Vercel Labs Team on Vercel. A member of the Team first needs to authorize it. |
NestJS projects may use SWC to compile TypeScript to either ESM or CommonJS. When compiling to CJS, the workflow builder's ESM steps bundle fails to import named exports from SWC-compiled CJS files because cjs-module-lexer can't detect SWC's _export() wrapper pattern. This change makes the module type configurable (default: 'es6'): - builder.ts: Add `moduleType` option to NestBuilderOptions. When set to 'commonjs', rewrite externalized .ts imports in steps.mjs to use require() via createRequire, avoiding ESM/CJS interop issues. - cli.ts: Add `--module` flag to `init` command, allowing users to generate .swcrc with either 'es6' (default) or 'commonjs' module type. - package.json: Add 'default' export conditions for broader compatibility. Co-authored-by: Cursor <cursoragent@cursor.com>
boomyao
force-pushed
the
feat/nest-commonjs-support
branch
from
d2d7de8 to
a9c92cb
Compare
| this.#workingDir = workingDir; | ||
| } | ||
|
|
||
| get outDir(): string { |
There was a problem hiding this comment.
The regex only matches named imports (import { ... } from "..."), but what about:
- Default imports:
import foo from "../something.ts"— unlikely in practice for externalized imports but worth noting. - Namespace imports:
import * as foo from "../something.ts"— this pattern could appear in generated bundles. - Side-effect imports:
import "../something.ts"— probably not an issue since these don't have named exports.
Could you verify which import patterns esbuild actually emits in the externalized steps bundle? If it only ever generates named imports here, this is fine — just want to make sure we're not silently skipping rewrites for some patterns.
packages/nest/src/builder.ts
Outdated
| const stepsContent = await readFile(stepsPath, 'utf-8'); | ||
|
|
||
| // Match any import from a relative .ts file (externalized source imports) | ||
| const tsImportRegex = |
There was a problem hiding this comment.
The as -> : replacement (imports.replace(/\s+as\s+/g, ':')) handles destructuring aliases (import { foo as bar } -> const { foo: bar }), which is correct. However, this regex could potentially match the substring as inside identifier names if there are imports like import { hasValue as hv } — which would still work correctly since the regex requires surrounding whitespace. Good.
One edge case: if the import list has trailing commas or whitespace variations, the {${cjsImports}} output should still be valid JS. Might be worth verifying this works with esbuild's actual output formatting.
packages/nest/src/builder.ts
Outdated
| const relToWorkingDir = relative(this.#workingDir, absSourcePath); | ||
|
|
||
| // Map source path to compiled path: | ||
| // Find which source dir this file belongs to, strip it, |
There was a problem hiding this comment.
When dirs includes "." (the root), prefix becomes "" and the if (prefix && ...) check will be false, so we fall through to the fallback which prepends distDir to the entire path. This means if someone uses dirs: ["."] with distDir: "dist", a file at services/foo.ts would be mapped to dist/services/foo.ts -> dist/services/foo.js, which seems correct.
However, if dirs has "." alongside other dirs like [".", "src"], the "." entry would never match in the loop (it's skipped), and "src" would match src/... files. Files outside src/ would fall through to the default. This seems to work correctly by accident but the logic flow is a bit subtle — maybe worth adding a comment explaining the "." case.
| // When the NestJS project compiles to CJS via SWC, the ESM steps bundle | ||
| // can't import named exports from CJS files because cjs-module-lexer | ||
| // doesn't recognize SWC's _export() wrapper pattern. | ||
| // Rewrite externalized .ts imports to use require() via createRequire. |
There was a problem hiding this comment.
The requireShim is prepended to the entire file content. If the original stepsContent starts with other imports (e.g., from @workflow/core or npm packages), the createRequire import and const require = ... will be at the very top, followed by the original content with .ts imports rewritten.
This should be fine since the shim uses import (hoisted) followed by const (not hoisted), and the rewritten require() calls are also const declarations. Just noting that this means the file will have a mix of import statements and const ... = require(...) calls. Node.js handles this correctly in .mjs files — import and require can coexist when require comes from createRequire.
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Review Summary: [nest] Support CommonJS compilation for NestJS projects
Thanks for the contribution, @boomyao! This is a well-targeted fix for a real interop problem between SWC's CJS output and Node.js's cjs-module-lexer.
What this PR does
Adds opt-in CommonJS support to @workflow/nest by:
- Adding
moduleTypeanddistDiroptions toNestBuilderOptionsand the CLI - Post-processing the ESM
steps.mjsbundle to rewrite.tsimports asrequire()calls viacreateRequirewhenmoduleType: 'commonjs'is set - Adding
defaultexport conditions topackage.jsonfor broader CJS compatibility
Strengths
- Opt-in design: Defaults remain
es6, so existing ESM users are completely unaffected - Well-documented: Good JSDoc comments, README updates, and inline code comments explaining the "why"
- Correct approach: Using
createRequirein.mjsis the standard way to load CJS modules from ESM whencjs-module-lexerfails - Clean separation: The CJS rewrite is a self-contained post-processing step that doesn't touch the core build pipeline
Concerns & Questions
-
Regex coverage (see inline comment): The import regex only matches
import { ... } from "...ts"— are namespace imports (import * as) possible in esbuild's externalized output? -
Missing tests: The PR has no automated tests. Given the regex-based string rewriting, this would benefit from at least unit tests for:
#rewriteStepsBundleForCjs()with sample bundle content#mapSourceToDistPath()with variousdirsconfigurations- The CLI's
parseModuleType()function
-
Missing changeset: Per the repo's contribution guidelines, a changeset is needed. Please run
pnpm changeset addand include@workflow/nestas a patch change. -
.tsxfiles: The regex matches.tsextensions only. If someone has.tsxworkflow files, would those also be externalized with.tsxextensions? If so, the regex should also handle.tsx. -
Fragility of string rewriting: The regex-based approach is inherently coupled to esbuild's output format. If esbuild changes how it formats externalized imports (e.g., different whitespace, semicolons, or import grouping), this could silently break. Consider adding a warning or error if the rewrite produces zero matches when
moduleType: 'commonjs'is set (i.e., the bundle has no.tsimports to rewrite — which might indicate a format change rather than "no externalized imports").
Verdict
The approach is sound and the code quality is good. The main blockers before merging would be:
- Adding unit tests for the rewrite logic
- Adding a changeset
- Clarifying the regex coverage question (namespace/default imports and
.tsx)
Nice work overall!
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Human here: this looks great and we're happy to add this. Can you follow these instructions for getting the DCO check to pass? That's the only real blocker for merging this
Replace hardcoded path assumptions (../../src/ → ../../dist/) with dynamic path resolution using resolve/relative from pathe. Add configurable `distDir` option so the builder works for any NestJS project, not just a specific directory layout. Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: yao <zhangyaoruo@outlook.com>
boomyao
force-pushed
the
feat/nest-commonjs-support
branch
from
e9d8cbd to
d5bbcb0
Compare
4 participants
Summary
NestJS projects may use SWC to compile TypeScript to either ESM or CommonJS. When compiling to CJS, the
@workflow/nestbuilder's ESM steps bundle fails to import named exports from SWC-compiled CJS files becausecjs-module-lexercannot detect SWC's_export()wrapper pattern.This PR adds configurable module type support to
@workflow/nest, keepinges6as the default while allowing users to opt intocommonjs:builder.ts: AddmoduleTypeoption ('es6' | 'commonjs') toNestBuilderOptions. When set to'commonjs', the builder rewrites externalized.tsimports insteps.mjsto userequire()viacreateRequire, avoiding ESM/CJS named-export interop issues.cli.ts: Add--module <type>flag to theinitcommand, so users can generate.swcrcwith eitheres6(default) orcommonjsmodule type. Example:npx @workflow/nest init --module commonjspackage.json: Adddefaultexport conditions for broader compatibility with CJS consumers.Usage
Builder API:
CLI:
Background
When NestJS uses SWC with
module.type: 'es6', the compiled output is ESM. However, some NestJS projects usemodule.type: 'commonjs'(Node.js default). In that case, when the ESM steps bundle (.mjs) tries toimport { ... } froma SWC-compiled CJS file, Node.js usescjs-module-lexerto detect named exports — but SWC's CJS wrapper pattern (_export()) is not recognized by the lexer, causing import failures.The fix uses
createRequireto load externalized CJS dependencies viarequire(), which correctly handles all CJS export patterns. This is only activated when the user explicitly opts intomoduleType: 'commonjs'.Test plan
@workflow/nestwith default ESM compilation (no behavior change)@workflow/nestwithmoduleType: 'commonjs'