Skip to content

parser: reject ES module syntax in .cjs/.cts files at runtime - #33895

Closed
robobun wants to merge 5 commits into
mainfrom
farm/1f118acb/cjs-esm-syntax-error
Closed

robobun wants to merge 5 commits into
mainfrom
farm/1f118acb/cjs-esm-syntax-error

Conversation

@robobun

@robobun robobun commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

The .cjs/.cts extension is unconditionally CommonJS in Node, regardless of package.json or file content: export, import statements, and top-level await are a SyntaxError there. Bun was classifying these files by content instead, so a .cjs file containing export const x = 7 was silently run as an ES module. A package could ship a .cjs that passed Bun-based CI and was unloadable in Node, with nothing telling the author their .cjs wasn't what its extension said.

// b.cjs
export const x = 7;
$ node b.cjs
SyntaxError: Unexpected token 'export'

$ bun b.cjs        # before: silently ran as ESM, exit 0
$ bun b.cjs        # after:
error: Cannot use 'export' in a CommonJS module
note: This file is CommonJS because it has the ".cjs" extension. Rename it to end in .mjs to use ES module syntax.

Cause

In js_parser::parse_entry, p.esm_export_keyword.len > 0 short-circuited to ExportsKind::Esm before p.options.module_type was consulted, and the ModuleType::Cjs arm fell back to ExportsKind::Esm when has_es_module_syntax was set. The extension contract was never enforced.

This surfaced two more gaps on the off-thread transpile path:

  • RuntimeTranspilerStore::transpile derived module_type only from the nearest package.json "type" field, not the file extension, so an imported .cjs saw ModuleType::Unknown. A .cjs with no module/exports/require references loaded via import was then sniffed as ESM (module was undefined at runtime).
  • Parser errors pushed into log while still returning a well-formed AST were dropped by AsyncModule::fulfill when parse_error was None.

Fix

  • parse_entry.rs: when module_type == Cjs, commonjs_at_runtime, not bundling, and the file is .cjs/.cts, emit a ranged error at the first export/import/await keyword. Scoped to the extension (not all of ModuleType::Cjs) because Bun has long allowed export in .js/.ts under "type": "commonjs" and packages depend on it; the bundler continues to accept mixed input.
  • RuntimeTranspilerStore.rs / jsc_hooks.rs: thread module_type (already computed from extension + package.json) through to the off-thread parse job instead of re-deriving from ResolvedSourceTag.
  • RuntimeTranspilerStore.rs: set parse_error when log.errors > 0 after a successful parse so the error reaches the import promise.
  • parse_entry.rs / RuntimeTranspilerCache.rs: include module_type in the runtime transpiler hash and bump the cache version, since byte-identical sources now produce different results per module_type.

Two existing fixtures that relied on the old behavior were updated: require-extensions.test.ts's "secretly sync esm" case now uses a .custom extension (its intent is to test require.extensions wrapping, not the .cjs contract), and the unreferenced module-type-fixture/*/import.cjs fixtures were removed.

This overlaps with #33807 on the RuntimeTranspilerStore plumbing and the features-hash addition; whichever lands second needs a trivial rebase there.

How did you verify your code works?

test/cli/run/run-cjs.test.ts covers .cjs and .cts as entrypoint, via import *, via await import(), via require(), with export const/export default/bare import/named import/top-level await, plus negative controls (dynamic import() expression still allowed, .js under "type":"commonjs" with export still accepted, marker-less .cjs imported as CommonJS). 11 of 15 fail on the released binary and all 15 pass on this branch.

The .cjs/.cts extension is unconditionally CommonJS in Node; 'export',
'import' statements and top-level 'await' are a SyntaxError there. Bun
was classifying such files by content and silently running them as ES
modules, so a package could ship a .cjs that was unloadable in Node.

Also threads module_type through the off-thread transpiler (previously
only the package.json 'type' field reached it, not the extension) and
surfaces parser log errors that were dropped on that path when parse
returned a well-formed AST.
@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 1 minute

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e0736c69-2240-45ce-b4f2-14cffb65a091

📥 Commits

Reviewing files that changed from the base of the PR and between 8594007 and 2f35b30.

📒 Files selected for processing (2)
  • src/js_parser/parse/parse_entry.rs
  • test/cli/run/run-cjs.test.ts

Walkthrough

Changes

The parser now rejects ESM syntax in runtime CommonJS .cjs and .cts files, preserves TypeScript export parsing state, propagates explicit module types through runtime transpiler jobs, invalidates older cache entries, and expands related CLI and module-loading tests.

CommonJS runtime transpilation

Layer / File(s) Summary
Parser ESM detection and state restoration
src/js_parser/parse/parse_stmt.rs, src/js_parser/parse/parse_entry.rs
CommonJS runtime parsing reports ESM syntax in .cjs and .cts files, while TypeScript-only export branches restore esm_export_keyword.
Runtime transpiler module-type propagation
src/jsc/RuntimeTranspilerStore.rs, src/runtime/jsc_hooks.rs, src/js_parser/parse/parse_entry.rs, src/jsc/RuntimeTranspilerCache.rs
Transpiler jobs receive explicit module types, propagate them to parse options, reject parses with logged errors, and use a new cache version and module-type hash input.
CommonJS extension behavior coverage
test/cli/run/run-cjs.test.ts, test/cli/run/module-type-fixture/esm/import.cjs, test/cli/run/run-detect-module-type.ts
CLI tests cover rejected and allowed syntax across .cjs and .cts, and adjust module-type fixtures and expectations.
Custom require extension regression coverage
test/js/node/module/require-extensions.test.ts
The synchronous ESM test uses a .custom extension handler while retaining its module and compilation assertions.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and accurately summarizes the main change: runtime rejection of ES module syntax in .cjs/.cts files.
Description check ✅ Passed The description includes the required purpose and verification sections and provides substantive details on the behavior change and testing.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Jul 10, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 10:28 PM PT - Jul 9th, 2026

@robobun, your commit 2f35b30 is building: #71369

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. parser: .mjs/"type":"module" is authoritative over module/exports identifier refs #33807 - Both PRs thread module_type through RuntimeTranspilerStore, add it to hash_for_runtime_transpiler, and bump the transpiler cache version, modifying the same four files (parse_entry.rs, RuntimeTranspilerStore.rs, RuntimeTranspilerCache.rs, jsc_hooks.rs) for the same root cause: the off-thread transpiler re-derived module type from ResolvedSourceTag instead of the extension-aware value

🤖 Generated with Claude Code

@robobun

robobun commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Not a duplicate: #33807 makes .mjs/"type":"module" authoritative over the module/exports identifier sniff (CJS-looking file that should be ESM). This PR makes .cjs/.cts authoritative over export/import syntax (ESM-looking file that must be CJS). Opposite directions of the same classifier.

They share the RuntimeTranspilerStore plumbing and features-hash change because both need module_type to reach the off-thread parser; that overlap is already noted in the PR body. Whichever lands second needs a trivial rebase on those four lines.

Comment thread src/js_parser/parse/parse_entry.rs Resolved
robobun and others added 2 commits July 10, 2026 05:21
….cts

The new check used esm_import_keyword/esm_export_keyword, which are also
set for constructs that aren't ES-module statement syntax: type-only
imports/exports (import type, export type, export interface, export
declare, export {type X}) and import.meta. A .cts containing
'import type {X} from ...; module.exports = 1' or a .cjs using
import.meta.dir (which Bun's CJS wrapper explicitly supports) now
errored incorrectly.

For imports, detect a real 'import' statement via its import record
(type-only imports and import.meta don't create one). For exports,
restore esm_export_keyword to its previous value at the type-only
S::TypeScript returns in t_export, matching the existing restore for
'export = value'.
Comment thread src/js_parser/parse/parse_entry.rs Resolved
'export import X = require(...)' is intentionally not whitelisted: Bun
transpiles it to 'export const X = require(...)', a real ESM export in
the printed output, so the new error is accurate for Bun's output and
matches Node (which rejects import-equals in strip-only mode).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/js_parser/parse/parse_stmt.rs`:
- Around line 914-922: Update the export handling branch for
StmtIdentifier::SNamespace, SAbstract, and SModule to restore esm_export_keyword
after p.parse_stmt(opts) when parsing produces StmtData::STypeScript, including
empty or type-only bodies. Preserve the keyword for runtime statements, and
ensure .cjs/.cts no-op namespace or module declarations do not trigger CommonJS
export errors.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4c8aef9d-9137-4124-81d0-9bae7c23e1cf

📥 Commits

Reviewing files that changed from the base of the PR and between 6bb5135 and 8594007.

📒 Files selected for processing (11)
  • src/js_parser/parse/parse_entry.rs
  • src/js_parser/parse/parse_stmt.rs
  • src/jsc/RuntimeTranspilerCache.rs
  • src/jsc/RuntimeTranspilerStore.rs
  • src/runtime/jsc_hooks.rs
  • test/cli/run/module-type-fixture/cjs/import.cjs
  • test/cli/run/module-type-fixture/esm/import.cjs
  • test/cli/run/run-cjs.test.ts
  • test/cli/run/run-detect-module-type.ts
  • test/js/node/module/extensions-fixture/secretly_esm.custom
  • test/js/node/module/require-extensions.test.ts
💤 Files with no reviewable changes (3)
  • test/cli/run/module-type-fixture/cjs/import.cjs
  • test/cli/run/module-type-fixture/esm/import.cjs
  • test/cli/run/run-detect-module-type.ts

Comment on lines 914 to +922
StmtIdentifier::SNamespace
| StmtIdentifier::SAbstract
| StmtIdentifier::SModule
| StmtIdentifier::SInterface => {
| StmtIdentifier::SModule => {
// "export namespace Foo {}"
// "export abstract class Foo {}"
// "export module Foo {}"
opts.is_export = true;
return p.parse_stmt(opts);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map the relevant parser file and the namespace parser implementation.
ast-grep outline src/js_parser/parse/parse_stmt.rs --view expanded || true
printf '\n---\n'
ast-grep outline src/js_parser/parse/parse_type_script_namespace_stmt.rs --view expanded || true
printf '\n---\n'
rg -n "parse_type_script_namespace_stmt|STypeScript|esm_export_keyword|SNamespace|SModule|SAbstract" src/js_parser/parse -S

Repository: oven-sh/bun

Length of output: 8876


🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '169,380p' src/js_parser/parse/parse_typescript.rs
printf '\n--- parse_stmt export branches ---\n'
sed -n '840,990p' src/js_parser/parse/parse_stmt.rs
printf '\n---\n'
sed -n '990,1135p' src/js_parser/parse/parse_stmt.rs

Repository: oven-sh/bun

Length of output: 24241


Restore esm_export_keyword when export namespace / export module erases to TS-only. These forms can return StmtData::STypeScript for empty or type-only bodies, but this branch returns p.parse_stmt(opts) without resetting the export keyword. In .cjs/.cts files that turns a no-op namespace into a CommonJS export error.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js_parser/parse/parse_stmt.rs` around lines 914 - 922, Update the export
handling branch for StmtIdentifier::SNamespace, SAbstract, and SModule to
restore esm_export_keyword after p.parse_stmt(opts) when parsing produces
StmtData::STypeScript, including empty or type-only bodies. Preserve the keyword
for runtime statements, and ensure .cjs/.cts no-op namespace or module
declarations do not trigger CommonJS export errors.

Comment on lines 914 to +921
StmtIdentifier::SNamespace
| StmtIdentifier::SAbstract
| StmtIdentifier::SModule
| StmtIdentifier::SInterface => {
| StmtIdentifier::SModule => {
// "export namespace Foo {}"
// "export abstract class Foo {}"
// "export module Foo {}"
opts.is_export = true;
return p.parse_stmt(opts);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 Two more over-match cases in the same class as the open comment on export import X = require(...), at different t_export() arms that a fix to only the T::TImport arm would miss: the SNamespace | SAbstract | SModule arm here and the T::TEnum arm at ~line 851 both recurse without restoring esm_export_keyword. A .cts containing export namespace Foo { export type X = number } (type-only, erased by tsc — directly analogous to export interface, which the fix commit did handle) or export enum Foo { A = 1 } (tsc emits exports.Foo = ... for .cts) now fails with "Cannot use 'export' in a CommonJS module … Rename to .mts", regressing files that ran before this PR. The type-only namespace case is unambiguously valid .cts; export enum / value-emitting export namespace are a design decision, but the current diagnostic is misleading either way.

Extended reasoning...

What the bug is

The fix commit 8594007 addressed the resolved comment by restoring esm_export_keyword at every t_export() path that returns S::TypeScript{}, and by splitting StmtIdentifier::SInterface/SDeclare out to restore before recursing. But it left two sibling arms untouched that also recurse without restoring, and neither is covered by the still-open comment on export import X = require(...):

  • StmtIdentifier::SNamespace | SModule (parse_stmt.rs:914-921), grouped with SAbstract: sets opts.is_export = true and recurses to parse_type_script_namespace_stmt. esm_export_keyword is never restored, regardless of whether the namespace is type-only (returns S::TypeScript{}) or value-emitting (returns S::Namespace).
  • T::TEnum (~line 845-851): sets opts.is_export = true and recurses to parse_typescript_enum_stmt, which returns a value-emitting S::Enum. esm_export_keyword is never restored.

Step-by-step proof (type-only namespace)

Take foo.cts:

export namespace Foo { export type X = number }
module.exports = 1;
  1. t_export() at parse_stmt.rs:803 sets p.esm_export_keyword = p.lexer.range() (opts.is_module_scope is true).
  2. Token is TIdentifier namespace → matches the SNamespace | SAbstract | SModule arm at line 914, which sets opts.is_export = true and does return p.parse_stmt(opts) without restoring esm_export_keyword.
  3. The recursion reaches parse_stmt_fallthrough → TsStmtNamespace → parse_type_script_namespace_stmt (parse_typescript.rs). Inside the body, the inner export type X = number parses with is_namespace_scope: true, is_module_scope: false (so line 803 does not overwrite esm_export_keyword), returns S::TypeScript{}, and parse_stmts_up_to filters it out.
  4. stmts.len() == 0 == import_equal_count, so parse_type_script_namespace_stmt returns S::TypeScript{} — the namespace is erased exactly like an interface. esm_export_keyword is still set.
  5. In parse_entry.rs: module_type == Cjs (from the .cts extension), commonjs_at_runtime true, ext is .cts, esm_export_keyword.len > 0 → emits Cannot use 'export' in a CommonJS module.

Before this PR the same file worked (classified as ESM, but the namespace erased to nothing and module.exports still ran). This is the exact analogue of export interface Foo {}, which the fix commit did split out to restore — a type-only (uninstantiated) namespace is erased by tsc and leaves valid CommonJS.

Step-by-step proof (export enum)

Take foo.cts:

export enum Foo { A = 1 }
  1. t_export() sets p.esm_export_keyword (line 803).
  2. T::TEnum arm (~line 845): opts.is_export = true; p.parse_stmt(opts) — no restore.
  3. Recurses → t_enum() → parse_typescript_enum_stmt → returns S::Enum (value-emitting, not S::TypeScript{}).
  4. esm_export_keyword.len > 0 survives to the new check → Cannot use 'export' in a CommonJS module … Rename to .mts.

Before this PR this ran (as ESM). export enum in .cts is a real-world pattern for sharing constants between CJS packages; tsc compiles it to exports.Foo = .... The value-emitting export namespace Foo { export const x = 1 } case follows the same path via the SNamespace arm (returns S::Namespace since stmts.len() > import_equal_count).

Why nothing catches this

The negative-test matrix added in run-cjs.test.ts covers export type/export interface/export declare/export {type X} but not export namespace, export module, or export enum. The resolved PR comment enumerated export interface etc.; the unresolved PR comment covers export import X = require(...) (the T::TImport arm). These are different code sites — the SNamespace|SModule arm at line 914 and the T::TEnum arm at ~line 851 — that neither comment nor the fix commit touch.

On the value-emitting cases

The type-only namespace case is unambiguously valid .cts (same class as export interface). export enum and value-emitting export namespace are more nuanced: tsc under --verbatimModuleSyntax rejects them in .cts, and Bun's enum/namespace lowering currently prints export var Foo (not exports.Foo = ...), so simply classifying them as CJS wouldn't work without new lowering. Rejecting them may be defensible — but as-is it's an undocumented regression of previously-running code with a factually misleading diagnostic ("Rename to .mts" for TS-only syntax that default-mode tsc compiles to CJS in a .cts). If the intent is to reject them, they should get a distinct message and a test.

How to fix

At minimum, restore p.esm_export_keyword = previous_export_keyword in the SNamespace | SModule arm (splitting SAbstract out, since export abstract class is a value ESM export) so type-only namespaces pass. For export enum and value-emitting namespaces, either apply the same restore (matching the open export import = require comment) or emit a distinct diagnostic — and add negative tests for whichever decision is made:

["export namespace (type-only)", "export namespace Foo { export type X = number }\nmodule.exports = 1;\n"],
["export enum", "export enum Foo { A = 1 }\nmodule.exports = Foo;\n"],

Comment on lines +911 to +931
p.esm_export_keyword = previous_export_keyword; // This wasn't an ESM export statement after all
return Ok(p.s(S::TypeScript {}, loc));
}
StmtIdentifier::SNamespace
| StmtIdentifier::SAbstract
| StmtIdentifier::SModule
| StmtIdentifier::SInterface => {
| StmtIdentifier::SModule => {
// "export namespace Foo {}"
// "export abstract class Foo {}"
// "export module Foo {}"
opts.is_export = true;
return p.parse_stmt(opts);
}
StmtIdentifier::SInterface => {
// "export interface Foo {}"
p.esm_export_keyword = previous_export_keyword; // This wasn't an ESM export statement after all
opts.is_export = true;
return p.parse_stmt(opts);
}
StmtIdentifier::SDeclare => {
// "export declare class Foo {}"
p.esm_export_keyword = previous_export_keyword; // This wasn't an ESM export statement after all

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 The new esm_export_keyword = previous_export_keyword restores in t_export() are not gated on .cts/module_type/!bundle, so they change module classification for all TS files, not just .cts. A .ts file under "type":"commonjs" containing only export interface Foo {} was previously classified ExportsKind::Esm (implicit strict mode) and is now wrapped as CommonJS with no implicit strict mode; the bundler's is_file_considered_to_have_esm_exports also flips. This is arguably the more-correct behavior (type-only exports emit nothing), but it's a cross-cutting change beyond the PR's stated ".cjs/.cts at runtime" scope with no test coverage — worth either a test asserting the new classification or a note in the PR description.

Extended reasoning...

What changed

The eight new p.esm_export_keyword = previous_export_keyword restores in parse_stmt.rs (at the export as namespace, export type, export interface, export declare, export default <type-only>, and export {type X} returns) run unconditionally — they are not gated on .cts, module_type == Cjs, or !p.options.bundle. Before this PR, esm_export_keyword was restored only for export = value (the pre-existing T::TEquals arm at parse_stmt.rs:1350), matching esbuild.

What consumes esm_export_keyword

esm_export_keyword feeds several downstream classifiers that are unrelated to the new .cts check in parse_entry.rs:

  • parse_entry.rs:1684 — exports_kind = Esm when esm_export_keyword.len > 0
  • p.rs:3057-3059 — is_file_considered_to_have_esm_exports (consumed at visit_expr.rs:1910 for the bundler's direct-eval() handling)
  • p.rs:3064-3067 — has_es_module_syntax |= esm_export_keyword.len > 0
  • p.rs:3122-3124 — ImplicitStrictModeExport (implicit strict mode)
  • p.rs:8764 — ast.export_keyword

The visit pass (which reads/writes these at p.rs:3057+) runs before the exports-kind classifier at parse_entry.rs:1684, so all of them observe the restored (empty) range.

Step-by-step proof

Take foo.ts under "type": "commonjs" containing only:

export interface Foo {}

Before this PR: SInterface was grouped with SNamespace/SAbstract/SModule (no restore) → t_export() sets esm_export_keyword at line 803 and never restores it → p.rs:3122 sets ImplicitStrictModeExport, p.rs:3066 sets has_es_module_syntax = true, p.rs:3057 sets is_file_considered_to_have_esm_exports = true → parse_entry.rs:1684 short-circuits to ExportsKind::Esm. File runs as ESM in implicit strict mode.

After this PR: SInterface arm restores esm_export_keyword = previous_export_keyword (i.e. Range::NONE) → p.rs:3066's OR-in contributes false, and type-only exports never set has_es_module_syntax directly (they return S::TypeScript{}) → has_es_module_syntax = false, no ImplicitStrictModeExport, is_file_considered_to_have_esm_exports = false → parse_entry.rs:1684 is false, :1686 is false, falls through to :1774 ModuleType::Cjs → has_es_module_syntax == false → ExportsKind::Cjs + WrapMode::BunCommonjs. File now runs wrapped as CommonJS with module/exports in scope and not in implicit strict mode.

The same applies to export type Foo = ..., export declare ..., export {type X}, export as namespace, and the export default paths that yield S::TypeScript. For ModuleType::Unknown (no package.json), exports_kind still defaults to Esm at line 1837, but the strict-mode inference (p.rs:3122) and is_file_considered_to_have_esm_exports still change. Because the restores are not !p.options.bundle-gated, the bundler's classification also changes.

Why nothing catches this

The PR's new tests only exercise .cts files (where the restore is required for the new check to work). No test covers a non-.cts TS file whose only export is type-only, and no existing test asserts the old classification.

Impact assessment

The affected surface — files whose only export is type-only, under "type":"commonjs" — is narrow, and the new behavior is arguably more correct: a file whose exports all erase to nothing has no ESM syntax after erasure, and under "type":"commonjs" should be CJS. I can't construct a case where the new behavior is clearly wrong rather than just different. But it is a real, observable, cross-cutting behavior change that diverges from esbuild's classification and is outside the PR's stated ".cjs/.cts at runtime" scope.

Suggested action

Either (a) add a test asserting the new classification for a .ts file with only type-only exports under "type":"commonjs" and note the broader change in the PR description, or (b) if minimizing blast radius is preferred, track a separate "has real ESM export" flag for the .cts check instead of mutating esm_export_keyword globally.

Comment on lines +1661 to +1679
if let Some((range, what)) = found {
p.log().add_range_error_with_notes(
Some(p.source),
range,
format!("Cannot use {} in a CommonJS module", what).into_bytes(),
Box::new([bun_ast::Data {
text: std::borrow::Cow::Owned(
format!(
"This file is CommonJS because it has the \"{}\" extension. Rename it to end in {} to use ES module syntax.",
bstr::BStr::new(ext),
rename_to,
)
.into_bytes(),
),
..Default::default()
}]),
);
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 A .cjs containing both an import statement and a CJS marker (e.g. import 'node:assert'; module.exports = 1;) now emits two errors on the same import record — the new ".cjs extension" error at line 1661, then the pre-existing "Cannot use import statement with CommonJS-only features" + "because 'module' was used" error at line 1765. The second note is misleading for .cjs (removing module.exports wouldn't make the import valid; the extension is authoritative). Gating the pre-existing check at 1692 on rename_to.is_none() (or equivalent) would keep the output to one clean error.

Extended reasoning...

What the bug is

The new .cjs/.cts check at parse_entry.rs:1661 pushes an error into log but does not early-return, set exports_kind, or otherwise signal that the file has already been diagnosed. Execution falls through to the pre-existing exports-kind classifier at lines 1682–1771. When a .cjs file contains both an import statement and a CommonJS marker (module, exports, top-level return, or with), the pre-existing check at line 1708 finds the same ImportKind::Stmt record and emits a second error at the same range.

Step-by-step trace

Take bad.cjs:

import 'node:assert';
module.exports = 1;
  1. module_type == Cjs (from .cjs), commonjs_at_runtime is true, ext is .cjs → rename_to = Some(".mjs").
  2. esm_export_keyword.len == 0 (no export), so the check falls through to import_stmt: p.import_records contains one ImportKind::Stmt record for 'node:assert' → found = Some((record.range, "an 'import' statement")).
  3. Line 1661 emits error Fix ?? operator  #1: "Cannot use an 'import' statement in a CommonJS module" + note "This file is CommonJS because it has the ".cjs" extension. Rename it to end in .mjs to use ES module syntax."
  4. Execution continues. is_deoptimized_commonjs() is false (module.exports = 1 doesn't populate commonjs_named_exports). esm_export_keyword.len == 0 and top_level_await_keyword.len == 0, so line 1684 is false.
  5. uses_module_ref is true → line 1686 branch taken → exports_kind = Cjs, wrap_mode = BunCommonjs.
  6. Line 1692–1705 iterates p.import_records with the identical filter and finds the same ImportKind::Stmt record.
  7. Line 1765 emits error Fix calling #private() functions in classes #2: "Cannot use import statement with CommonJS-only features" + note "Try require("node:assert") instead" + note "This file is CommonJS because 'module' was used".

Both errors go into log; both are printed.

Why the second error is now misleading

Before this PR, only error #2 fired for this input, and its note ("CommonJS because 'module' was used") was accurate — removing module.exports would have caused Bun to sniff the file as ESM and accept the import. After this PR, that note is misleading: removing module.exports still leaves the .cjs-extension error, because the extension is now authoritative. The two hints ("rename to .mjs" vs "try require()") also point at different remediations, though both are individually valid.

Why nothing prevents it

The new check is deliberately non-short-circuiting so the fall-through classifier still assigns exports_kind / wrap_mode correctly. But the fall-through classifier's own import-statement diagnostic at line 1708 has no visibility into whether the new check already fired — there is no flag threaded between them, and rename_to is scoped inside the new block.

Impact

Diagnostic-quality only. The file is correctly rejected either way (it was already rejected before this PR via error #2), and the primary (new, correct) error appears first. Nothing breaks if merged as-is.

How to fix

Skip the pre-existing import-statement diagnostic when the extension check already fired. One option: hoist rename_to (or a cjs_ext_error_emitted bool) out of the new block and gate line 1692's search on it being None/false. Alternatively, when the new check fires and esm_export_keyword.len == 0 && top_level_await_keyword.len == 0, set exports_kind = Cjs; wrap_mode = BunCommonjs; directly and skip the classifier entirely for .cjs/.cts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants