POC: support dynamic ESM imports in node-vm scripts#8394
Open
Cicolas wants to merge 8 commits into
Open
Conversation
Cicolas
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesDynamic import support
Estimated review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
also tested with in the pre-request script: const { faker } = await import('@faker-js/faker');
function makeUser() {
faker.seed(123);
return {
id: faker.datatype.uuid(),
name: faker.name.fullName(),
email: faker.internet.email().toLowerCase()
};
}
const user = makeUser(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 `@packages/bruno-js/src/sandbox/node-vm/index.js`:
- Around line 79-80: Dynamic import handling in node-vm currently bypasses
Bruno’s sandbox module policy by assigning
vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER in the vm setup. Update the sandbox
logic in the node-vm initializer so import() is either blocked for now or routed
through the same Bruno-controlled resolution policy used by require(), enforcing
additionalContextRootsAbsolute before any ESM module load. Use the existing
sandbox setup around scriptOptions.importModuleDynamically and the
module-loading path in the node-vm implementation to keep the behavior
consistent.
🪄 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: CHILL
Plan: Pro
Run ID: 64da074c-ebf7-4b6a-87b7-4deefa5933cf
📒 Files selected for processing (2)
packages/bruno-js/src/sandbox/node-vm/index.jspackages/bruno-js/src/sandbox/node-vm/index.spec.js
Contributor
There was a problem hiding this comment.
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 `@packages/bruno-app/src/utils/codemirror/javascript-lint.js`:
- Around line 17-25: The dynamic import check in isTopLevelDynamicImportError
only matches import( with no whitespace, so it can miss valid spaced forms.
Update the evidence check to accept optional whitespace between import and the
opening parenthesis while keeping the existing E033, a === 'import', and scope
=== '(main)' conditions. Use the same helper function to locate and adjust the
matching logic.
🪄 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: CHILL
Plan: Pro
Run ID: 6d3339dc-71bd-43c5-9feb-8b1be1e488cf
📒 Files selected for processing (2)
packages/bruno-app/src/utils/codemirror/javascript-lint.jspackages/bruno-app/src/utils/codemirror/javascript-lint.spec.js
ashuvssut
approved these changes
Jun 27, 2026
Contributor
There was a problem hiding this comment.
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 `@packages/bruno-app/src/utils/codemirror/javascript-lint.js`:
- Around line 73-80: The current E033 suppression in javascript-lint.js only
checks error.evidence on the import line, so multiline await/import() cases are
missed. Update the logic around the TOP_LEVEL_AWAIT_DYNAMIC_IMPORT_PATTERN check
to inspect surrounding source text or adjacent lines in the linting flow, using
the existing javascript-lint.js handling for JSHint errors and the E033/import
branch. Then add a test/spec covering the split-line form like await on one line
and import() on the next to verify the lint error is suppressed.
🪄 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: CHILL
Plan: Pro
Run ID: aa4272bc-3905-4668-b08e-43251c880d5e
📒 Files selected for processing (1)
packages/bruno-app/src/utils/codemirror/javascript-lint.js
Author
|
got rid of |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/bruno-js/src/sandbox/node-vm/esm-loader.js`:
- Around line 335-341: The synthetic module in esm-loader.js is re-exporting
CommonJS properties via SyntheticModule while also setting the synthetic default
export, so a CJS object’s own default key can overwrite it. Update the export
loop in the synthetic module callback to skip the default property when
iterating Object.keys(exportsValue), keeping setExport('default', exportsValue)
as the only default binding. Refer to the SyntheticModule setup and its
exportsValue/Object.keys handling to make the change safely.
In `@packages/bruno-js/src/sandbox/node-vm/index.spec.js`:
- Around line 850-875: The current test in node-vm/index.spec.js only covers a
CommonJS dependency being wrapped as a synthetic module, so it does not exercise
the true ESM path. Add a small node_modules fixture in the existing
runScriptInNodeVm test flow that represents a package with ESM syntax and a
package.json marked for module loading, then import it via the same dynamic
import assertion to verify shouldLoadAsEsm() and the collection dependency
resolution path. Use the existing test names and helpers such as
runScriptInNodeVm, collectionPath, and the dynamic import scenario to keep the
new coverage aligned with the PR objective.
🪄 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: CHILL
Plan: Pro
Run ID: 7c6cd5fb-ece6-408b-a552-1938ca98533f
📒 Files selected for processing (5)
packages/bruno-js/src/sandbox/node-vm/cjs-loader.jspackages/bruno-js/src/sandbox/node-vm/esm-loader.jspackages/bruno-js/src/sandbox/node-vm/index.jspackages/bruno-js/src/sandbox/node-vm/index.spec.jspackages/bruno-js/src/sandbox/node-vm/utils.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds proof-of-concept support for dynamic ESM imports in Bruno’s
node-vmruntime.Developer Mode scripts can now use
await import()to load ESM modules, including local.mjsfiles and package dependencies.Changes
node-vmscripts..mjshelper from a Bruno script..mjsfiles cannot be loaded throughrequire().Example
Tested manually with a Bruno collection using:
where faker-helper.mjs imports a package dependency:
This worked with a collection-local package.json dependency on @faker-js/faker.
Testing
Result:
Notes
This is intentionally scoped as a POC. Dynamic imports currently use Node’s default VM import loader. A follow-up can add a Bruno-aware ESM loader with parity to
createCustomRequire.The code editor keeps showing an error on(fixed onimportused as a function as you can see on this print:7cbe5572aef1a663f48b84df8519a6dd3a0a3f6b)CustomImport
createCustomImport, works similarly in spirit to [import-in-the-middle] (https://github.com/nodejs/import-in-the-middle), but scoped to Bruno’snode-vmruntime instead of using a process-wide Node loader hook.Node’s
vm.ScriptsupportsimportModuleDynamicallyas a function, which lets us customize howimport()is resolved and evaluated when user scripts call dynamic import. That gives Bruno a path to implement ESM support with the same guarantees ascreateCustomRequire.Related Issues
#3537
Summary by CodeRabbit
New Features
import()(includingawait import()) for.mjsmodules when running code in the Node VM sandbox.Bug Fixes
await import(...)after top-levelawait..mjsusage viarequire()with clearer failure behavior.Tests
await.