-
-
Notifications
You must be signed in to change notification settings - Fork 611
feat(commonjs): add externalBuiltinsRequire to stub external Node builtins when wrapped #1931
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
Changes from all commits
a77af35
41adfa1
7dd4935
736caa6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,9 @@ export default function commonjs(options = {}) { | |
| defaultIsModuleExports: defaultIsModuleExportsOption, | ||
| esmExternals | ||
| } = options; | ||
| const rawExternalBuiltinsRequire = options.externalBuiltinsRequire; | ||
| const externalBuiltinsRequireStrategy = | ||
| rawExternalBuiltinsRequire === 'stub' ? 'stub' : 'create-require'; | ||
|
Comment on lines
+47
to
+49
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The allowed values ( SuggestionIntroduce shared constants and use them for normalization here (and for validation below): // near the top
const EXTERNAL_BUILTINS_REQUIRE_DEFAULT = 'create-require';
const EXTERNAL_BUILTINS_REQUIRE_STRATEGIES = new Set(['create-require', 'stub']);
// ...
const rawExternalBuiltinsRequire = options.externalBuiltinsRequire;
const externalBuiltinsRequireStrategy =
EXTERNAL_BUILTINS_REQUIRE_STRATEGIES.has(rawExternalBuiltinsRequire)
? rawExternalBuiltinsRequire
: EXTERNAL_BUILTINS_REQUIRE_DEFAULT;Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this refactor. |
||
| const extensions = options.extensions || ['.js']; | ||
| const filter = createFilter(options.include, options.exclude); | ||
| const isPossibleCjsId = (id) => { | ||
|
|
@@ -212,6 +215,15 @@ export default function commonjs(options = {}) { | |
| 'The namedExports option from "@rollup/plugin-commonjs" is deprecated. Named exports are now handled automatically.' | ||
| ); | ||
| } | ||
| if ( | ||
| rawExternalBuiltinsRequire != null && | ||
| rawExternalBuiltinsRequire !== 'create-require' && | ||
| rawExternalBuiltinsRequire !== 'stub' | ||
| ) { | ||
| this.warn( | ||
| `${PLUGIN_NAME}: invalid externalBuiltinsRequire "${rawExternalBuiltinsRequire}", using "create-require"` | ||
| ); | ||
| } | ||
| requireResolver = getRequireResolver( | ||
| extensions, | ||
| detectCyclesAndConditional, | ||
|
|
@@ -264,7 +276,7 @@ export default function commonjs(options = {}) { | |
| if (isWrappedId(id, EXTERNAL_SUFFIX)) { | ||
| const actualId = unwrapId(id, EXTERNAL_SUFFIX); | ||
| if (actualId.startsWith('node:')) { | ||
| return getExternalBuiltinRequireProxy(actualId); | ||
| return getExternalBuiltinRequireProxy(actualId, externalBuiltinsRequireStrategy); | ||
| } | ||
| return getUnknownRequireProxy( | ||
| actualId, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| module.exports = { | ||
| description: | ||
| "uses 'stub' proxy for external node: builtins when configured, avoiding node:module import", | ||
| pluginOptions: { | ||
| strictRequires: true, | ||
| externalBuiltinsRequire: 'stub' | ||
| }, | ||
| context: { | ||
| __filename: __filename | ||
| } | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| // Ensure the transform computes `wrappedModuleSideEffects` for an external | ||
| // wrapped dependency by including a Node builtin `require()` inside a function. | ||
| function unused() { | ||
| // External Node builtin require – converted to an external proxy. | ||
| // When `externalBuiltinsRequire: 'stub'`, calling this will throw; we | ||
| // invoke it inside a try/catch below so the test can snapshot the emitted | ||
| // stub proxy without failing at runtime. | ||
| require('node:crypto'); | ||
| } | ||
|
|
||
| try { | ||
| unused(); | ||
| } catch (_err) { | ||
| // Expected: in this fixture we configure `externalBuiltinsRequire: 'stub'`, | ||
| // so calling the proxy's `__require()` throws. We swallow the error so the | ||
| // test can assert on the generated code (no `node:module` import) without | ||
| // failing at runtime. | ||
| } | ||
|
|
||
| module.exports = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this is not what I expected. There should be an option that has the behavior before #1909, which is to hoist the require like non
node:requires are done.While
createRequireis not supported, some builtin modules are supported in those runtimes. So stubbing all builtin modules would break some codes that were working.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sapphi-red could you please elaborate some more? the more information you can write down, the better the agent will understand the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1909 should be reverted and reapplied with the new option to ensure that the behavior is same when the option is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not what I had in mind. Charlie isn't likely to understand what that means in this context, and I'm not going to be doing that manually. Additionally that'll require bringing tests forward.
If you're not specific in a way an agent can understand, this isn't likely to move forward and the current behavior will stand. Please leave specifics on how Charlie can move in the direction you would like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. Copilot can understand it at least.
sapphi-red#1