nitrojs / nitrojs/nitro

v2 externalization treats ESM-looking CommonJS comments as executable syntax

Open
#4,613 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

v2
Dominant language
TypeScript
Stars
11.2k
Forks
899
Avg merge
2d 24m
Merged PRs (30d)
40

Description

Environment
  • Nitro 2.13.4 (v2 at e6ed0aa35a94e873e2e42487aec149238846225a)
  • mlly 1.8.2; Node 24.15.0; Linux
Minimal reproduction

Create a Node dependency named comment-syntax with package.json:

{ "name": "comment-syntax", "version": "1.0.0", "main": "index.js" }

Its index.js is valid CommonJS:

/* export default example */
module.exports = 42;

Import that dependency from a Nitro server route. Both the default and experimental.legacyExternals resolvers decline to externalize it because isValidNodeImport() counts the comment as ESM syntax by default. Direct Node loading succeeds.

The underlying validator behavior can be checked independently:

import { createRequire } from "node:module";
import { isValidNodeImport } from "mlly";

const require = createRequire(import.meta.url);
const entry = require.resolve("comment-syntax");
console.log(await isValidNodeImport(entry)); // false
console.log(await isValidNodeImport(entry, { stripComments: true })); // true
Expected behavior

Node externalization should examine executable syntax, ignoring comments while preserving explicit inlining and rejection of real mixed CommonJS/ESM code.

Proposed fix and TypeScript dependency

Pass { stripComments: true } in both v2 externals implementations. Regression tests exercise real CommonJS files with the real validator, preserving explicit inline rules and real mixed-syntax bundling. The two comment cases fail before this change and pass afterward.

This is one part of the TypeScript server-import failure reported in analogjs/analog#2457, currently worked around in analogjs/analog#2548. TypeScript 6.0.3 additionally contains ESM-looking diagnostic strings and identifier prefixes. Those require the separate mlly fix tracked in unjs/mlly#369.

The Nitro change alone is not claimed to fix TypeScript. Combined qualification uses a locally packed mlly build; the final TypeScript fix needs a released mlly version with token-aware detection. Nitro v3 is being qualified separately.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the two v2 externals implementations that call mlly's isValidNodeImport(), then inspect the regression tests for real CommonJS files and comment cases. Verify that comments are ignored while explicit inline rules and genuine mixed CommonJS/ESM behavior remain unchanged, and run the relevant externalization test suite.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
backend, build-system, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.