fix(core): use esm-aware node built-in externals#1552
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a810a3de8e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Switches Rslib’s Node.js built-in externalization for ESM node targets from a hardcoded built-ins externals list to Rspack’s externalEsmNodeBuiltin handling, and adds an integration fixture/test to verify CommonJS built-in semantics are preserved.
Changes:
- Enable
externalEsmNodeBuiltinforformat: 'esm'+target: 'node'viarspack.experiments.RslibPlugin. - Stop forcing
output.externals = nodeBuiltInModulesfor ESM node outputs (keep for non-ESM node outputs). - Add an integration test + fixture to validate
require('stream')behavior when consumed from ESM output.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/externals/index.test.ts | Adds integration coverage asserting CommonJS built-in semantics in ESM output. |
| tests/integration/externals/esm-node-builtin/src/send.cjs | Adds a CJS module using Node built-ins (node:util, stream) to validate inheritance semantics. |
| tests/integration/externals/esm-node-builtin/src/index.ts | ESM entry that imports the CJS module and re-exports values for assertions. |
| tests/integration/externals/esm-node-builtin/rslib.config.ts | Adds an ESM bundle config for the new integration fixture. |
| tests/integration/externals/esm-node-builtin/package.json | Marks the fixture package as ESM ("type": "module"). |
| packages/core/src/config.ts | Wires externalEsmNodeBuiltin and adjusts Node target externals behavior by format. |
| packages/core/tests/snapshots/config.test.ts.snap | Updates snapshots reflecting the new config shape/arguments. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Summary
Switch Rslib's ESM node builtin handling to Rspack's
externalEsmNodeBuiltinpath instead of forcingexternals: nodeBuiltInModules.This preserves CommonJS runtime semantics for built-ins like
require('stream')in ESM output and aligns Rslib with the default smart handling added in Rspack.Related Links
Checklist