-
Notifications
You must be signed in to change notification settings - Fork 0
feat(commonjs)!: revert PR #1909 and add requireNodeBuiltins option #1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: sapphi-red <[email protected]>
Co-authored-by: sapphi-red <[email protected]>
|
@copilot add |
Co-authored-by: sapphi-red <[email protected]>
Added 3 new test fixtures with
These test the default behavior where node: builtins are treated as regular externals without createRequire injection. Snapshots will need to be generated locally (non-CI environment). Commit: 92634ff |
|
@copilot re-run tests to generate the snapshot and commit it |
Co-authored-by: sapphi-red <[email protected]>
Snapshots generated successfully. All 285 tests passing (282 original + 3 new variant tests). Generated snapshots for:
Commit: bd43a20 |
Summary
Successfully implemented the
requireNodeBuiltinsoption to address issue rollup#1924 by reverting PR rollup#1909's automatic createRequire injection and making it opt-in.Completed Tasks
requireNodeBuiltins(boolean, default: false) to typesTest Coverage
Created 3 new test fixtures to verify default behavior (requireNodeBuiltins: false):
strict-requires-external-node-builtin-default- Tests dead code elimination without createRequirestrict-requires-auto-external-node-builtin-default- Tests hoisting with strictRequires: automodule-side-effects-external-node-builtin-wrapped-default- Tests side effects handlingAll snapshots generated and all tests passing (285/285).
Implementation Details
Option:
requireNodeBuiltins: boolean(default:false)Behavior:
false(default): External Node built-ins likenode:fsare treated as regular externals, avoiding thenode:moduledependency. This is safe for edge runtimes.true: UsescreateRequire(import.meta.url)for lazy loading, matching PR fix(commonjs): avoid hoisting dynamically required node: builtins under strictRequires rollup/plugins#1909 behavior. Addsnode:moduledependency.Files Modified:
packages/commonjs/types/index.d.ts- Added type definitionpackages/commonjs/src/index.js- Read option and apply conditionallypackages/commonjs/src/resolve-require-sources.js- Conditional special handlingpackages/commonjs/README.md- Comprehensive documentationTesting
Next Steps
Ready for maintainer review and merge.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.