feat: add --require run arg to support commonJS #31201
Conversation
fraidev
force-pushed
the
require
branch
6 times, most recently
from
40c6581 to
085e3df
Compare
fraidev
changed the title
--require run argumentThere was a problem hiding this comment.
Pull Request Overview
This pull request implements support for the --require flag in Deno, which allows loading CommonJS modules before the main module execution. This feature is Node.js-compatible and ensures that modules loaded via --require are always treated as CommonJS, regardless of their file extension.
- Added
--requireflag for preloading CommonJS modules - Integrated require modules into the CJS tracking system to force CommonJS resolution
- Updated all relevant commands (run, test, bench, compile) to handle require modules
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
cli/args/flags.rs |
Added --require flag definition and parsing logic |
cli/args/mod.rs |
Added methods to retrieve require modules and combined preload modules |
cli/factory.rs |
Passed require_modules to resolver factory options |
cli/tools/run/mod.rs |
Updated to use all_preload_modules() for combined module loading |
cli/tools/test/mod.rs |
Updated to use all_preload_modules() for combined module loading |
cli/tools/bench/mod.rs |
Updated to use all_preload_modules() for combined module loading |
cli/tools/compile.rs |
Updated to use all_preload_modules() for combined module loading |
cli/standalone/binary.rs |
Added require_modules serialization for standalone binaries |
cli/rt/run.rs |
Integrated require_modules into CjsTracker and preload execution |
cli/lsp/config.rs |
Initialized empty require_modules for LSP configuration |
cli/lib/standalone/binary.rs |
Added require_modules field to Metadata struct |
libs/resolver/factory.rs |
Added require_modules field to ResolverFactoryOptions |
libs/resolver/cjs/mod.rs |
Added require_modules tracking to force CommonJS resolution |
tests/specs/run/require_modules_node/* |
Test for require with node built-in modules |
tests/specs/run/require_modules_multiple/* |
Test for multiple require modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fraidev
force-pushed
the
require
branch
2 times, most recently
from
aa69f3b to
9a66cfb
Compare
cli/args/flags.rs
Outdated
| .long("require") | ||
| .value_name("FILE") | ||
| .action(ArgAction::Append) | ||
| .help("A list of CommonJS modules that will be executed before the main module regardless of their file extension") |
There was a problem hiding this comment.
I'm not 100% sure we should mention the "regardless of their file extension". @dsherret what do you think?
There was a problem hiding this comment.
Make sense, I removed this part
cli/args/mod.rs
Outdated
| let mut modules = IndexSet::new(); | ||
| for preload_specifier in self.flags.preload.iter() { | ||
| modules | ||
| .insert(resolve_url_or_path(preload_specifier, self.initial_cwd())?); | ||
| } | ||
|
|
||
| for require_specifier in self.flags.require.iter() { |
There was a problem hiding this comment.
Can you please explain this one? Why are you storing both --preload and --require modules in the same set? preload are ESM while require are CJS
There was a problem hiding this comment.
This was my solution to not add a new Vec argument in all the system where is using modules.
As we only need to know if a module is a CJS or ESM module at CjsTracker phase, and there we have the modules separated (preload_modules and require_modules) to check if the URL is CJS or ESM module.
https://github.com/denoland/deno/pull/31201/files/9a66cfbca46ba77fba3c37867a47b9043d6b3563#diff-30d2cefb89866d76c4d39015b82c80a6f8ae31b4e02894bc9f20d87caf7e8c3bR141
I can back with only preload_modules and require_modules and merge them when is need (like all_preload_modules) or just add a new argument for require_modules in every place. WDYT?
There was a problem hiding this comment.
Or maybe rename to something like preload_and_require_modules?
There was a problem hiding this comment.
Okay, now I understand where my confusion comes from. I'd say it's better to keep them separate - I completely missed the is_from_require argument in get_known_mode_with_is_script.
There was a problem hiding this comment.
Sure, I added a commit to pass preload and require modules separately
| Ok(modules) | ||
| } | ||
|
|
||
| pub fn require_modules(&self) -> Result<Vec<ModuleSpecifier>, AnyError> { |
There was a problem hiding this comment.
Okay, really confusing :D I need an explanation for the difference between all_preload_modules(), preload_modules() and require_modules() - feels like we only need to latter two?
| let require_modules = metadata | ||
| .require_modules | ||
| .iter() | ||
| .map(|key| root_dir_url.join(key).unwrap()) | ||
| .collect::<Vec<_>>(); |
| @@ -0,0 +1,2 @@ | |||
| console.log("require_first.js loading"); | |||
There was a problem hiding this comment.
Hmm, I might be missing something here - but this will not execute as a CommonJS, but ESM - ie. if you try to access __filename or require you'll get hit with ReferenceError. Is this the behavior Node has?
There was a problem hiding this comment.
__filename or require should work, this is working fine for me:
index.js:
const os = require("node:os");
console.log(`cjs preload: ${os.platform()}`);
console.log(__filename)
globalThis.wasPreloaded = true;
main.ts:
console.log(`module was preloaded: ${globalThis.wasPreloaded}`);
Run and Output:
denod run --require index.js main.ts $!? require
cjs preload: darwin
/Users/frai/workspace/deno/deno/cjs.js
module was preloaded: true
(denod = alias for ./target/debug/deno)
There was a problem hiding this comment.
Should I add more test cases using it?
There was a problem hiding this comment.
Yes, having tests that call require() and/or __filename and __dirname would be useful, thanks
There was a problem hiding this comment.
Sure, I merged both tested in one to use multiples require modules and using require(), __filename and __dirname
fraidev
force-pushed
the
require
branch
5 times, most recently
from
60998dd to
102d2c0
Compare
bartlomieju
left a comment
bartlomieju
left a comment
There was a problem hiding this comment.
Almost there, just a few more changes
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new --require CLI option and threads its values (require_modules) through argument parsing, resolver/CJS tracking, worker factories, runtime execution, tooling, standalone metadata, and tests to preload required modules before the main script. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Parser
participant Resolver as ResolverFactory
participant CJS as CjsTracker
participant WorkerFactory as Worker Factory
participant Worker as Main Worker
participant Loader as Module Loader
User->>CLI: deno run --require r1.js --require r2.js main.js
CLI->>CLI: parse --require flags -> Flags.require
CLI->>Resolver: require_modules() -> resolve to ModuleSpecifiers
Resolver->>CJS: initialize CjsTracker(require_modules)
CJS->>CJS: mark require_modules for resolution
Resolver->>WorkerFactory: create_main_worker(preload, require_modules, ...)
WorkerFactory->>Worker: construct worker(with require_modules)
Worker->>Loader: execute_preload_modules() and execute_require_modules()
rect rgba(220,240,220,0.6)
note right of Loader: For each module: load -> evaluate
Loader->>CJS: resolution checks consider is_from_require
end
Loader->>Worker: setup complete
Worker->>Worker: run main.js
Worker->>User: stdout / exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
cli/rt/run.rs (1)
852-868: Require-module wiring is correct; consider de‑duplicating computationThe new
require_moduleshandling correctly:
- Resolves
metadata.require_modulesagainstroot_dir_url.- Passes the list into
CjsTracker::newso those modules are always treated as CJS.- Passes a (recomputed) list into
create_main_workerso the worker factory can preload them.Functionally this is fine, but you’re computing
require_modulestwice with identical logic and shadowing the name. Consider computing it once and either cloning for theCjsTrackeror passing byArc/shared slice to both call sites to avoid drift if this mapping changes later.Also applies to: 1092-1104
cli/args/flags.rs (3)
4163-4165: Scope--requireto runtime subcommands (avoid exposing it on resolution-only commands)Wiring via
compile_args_without_check_args/compile_args_without_check_parsemakes--requireappear oncheck,doc,cache,fmt,coverage,info, etc., which don't execute code. This can confuse users and doesn’t add value. Prefer scoping to runtime-only paths (run,serve,test,bench,eval,compile) by moving the arg and its parser into the runtime args.Suggested changes:
- Remove from compile args…
--- a/cli/args/flags.rs @@ - .arg(require_arg())--- a/cli/args/flags.rs @@ - require_arg_parse(flags, matches);
- …and add to runtime args:
--- a/cli/args/flags.rs @@ fn runtime_misc_args(app: Command) -> Command { app .arg(cached_only_arg()) .arg(location_arg()) .arg(v8_flags_arg()) .arg(seed_arg()) .arg(enable_testing_features_arg()) .arg(trace_ops_arg()) - .arg(eszip_arg()) + .arg(eszip_arg()) + .arg(require_arg()) }--- a/cli/args/flags.rs @@ fn runtime_args_parse( env_file_arg_parse(flags, matches); trace_ops_parse(flags, matches); eszip_arg_parse(flags, matches); + require_arg_parse(flags, matches); Ok(()) }This keeps
--requirewhere it actually has effect, reduces surface area, and aligns with user expectations.Also applies to: 6527-6529
4754-4764: Clarify help to state execution orderTiny UX nit: mention order to match Node users’ expectations.
- .help( - "A list of CommonJS modules that will be executed before the main module", - ) + .help("CommonJS modules to run before the main module (in the order specified)")
13487-13533: Add a guardrail test for the-rshort flag confusionGiven
-ris already used by--reload, a quick test thatdeno run -r setup.js main.tsfails with a helpful message (or at least does not setFlags.require) would reduce user confusion when migrating from Node. If you adopt the scoping change above, also consider a negative test thatdeno check --require xis rejected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/specs/run/require_modules_multiple/multiple.outis excluded by!**/*.out
📒 Files selected for processing (25)
cli/args/flags.rs(6 hunks)cli/args/mod.rs(1 hunks)cli/factory.rs(1 hunks)cli/lib/standalone/binary.rs(1 hunks)cli/lib/worker.rs(5 hunks)cli/lsp/config.rs(1 hunks)cli/lsp/testing/execution.rs(1 hunks)cli/rt/run.rs(3 hunks)cli/standalone/binary.rs(2 hunks)cli/tools/bench/mod.rs(10 hunks)cli/tools/compile.rs(1 hunks)cli/tools/deploy.rs(1 hunks)cli/tools/jupyter/mod.rs(1 hunks)cli/tools/lint/plugins.rs(1 hunks)cli/tools/repl/mod.rs(1 hunks)cli/tools/run/mod.rs(10 hunks)cli/tools/serve.rs(2 hunks)cli/tools/test/mod.rs(10 hunks)cli/worker.rs(4 hunks)libs/resolver/cjs/mod.rs(6 hunks)libs/resolver/factory.rs(2 hunks)tests/specs/run/require_modules_multiple/__test__.jsonc(1 hunks)tests/specs/run/require_modules_multiple/main_multiple.ts(1 hunks)tests/specs/run/require_modules_multiple/require_first.js(1 hunks)tests/specs/run/require_modules_multiple/require_second.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
cli/tools/compile.rs (2)
cli/factory.rs (1)
cli_options(375-386)cli/standalone/binary.rs (2)
cli_options(719-724)cli_options(726-731)
cli/standalone/binary.rs (2)
cli/args/mod.rs (1)
require_modules(653-664)libs/config/workspace/mod.rs (1)
root_dir_url(477-479)
cli/lib/worker.rs (1)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)
libs/resolver/cjs/mod.rs (1)
cli/args/mod.rs (1)
require_modules(653-664)
cli/args/flags.rs (2)
ext/node/polyfills/01_require.js (2)
require(1031-1031)require(1144-1146)tests/specs/bundle/main/uses_node_builtin.cjs (1)
require(1-1)
cli/rt/run.rs (3)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)libs/config/workspace/mod.rs (1)
root_dir_url(477-479)cli/lib/worker.rs (1)
main_module(827-829)
libs/resolver/factory.rs (1)
ext/node/polyfills/url.ts (1)
Url(160-912)
cli/tools/test/mod.rs (2)
cli/args/mod.rs (2)
preload_modules(640-651)require_modules(653-664)cli/standalone/binary.rs (2)
cli_options(719-724)cli_options(726-731)
cli/tools/run/mod.rs (3)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)cli/factory.rs (1)
cli_options(375-386)cli/standalone/binary.rs (2)
cli_options(719-724)cli_options(726-731)
cli/worker.rs (1)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)
cli/tools/bench/mod.rs (2)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)cli/standalone/binary.rs (2)
cli_options(719-724)cli_options(726-731)
🔇 Additional comments (29)
cli/tools/lint/plugins.rs (1)
160-161: LGTM! Clear documentation for unsupported feature.The empty vector for require modules is appropriate here, and the comment clearly explains that
deno lintdoesn't support this feature, matching the pattern used for preload modules.cli/tools/serve.rs (1)
103-103: LGTM! Consistent placeholder for future feature.Both call sites correctly pass empty vectors for require modules, matching the pattern used for preload modules. The existing TODO comments indicate these parameters may be populated in future work.
Also applies to: 159-159
cli/lib/worker.rs (2)
586-586: LGTM! Clean API extension for require modules.The
require_modulesparameter is properly threaded through the worker creation API:
- Added to public method signatures with correct type (
Vec<Url>)- Passed through to underlying
create_custom_workerimplementation- Stored in
LibMainWorkerstruct for execution during preload phaseThe API changes are consistent and well-structured.
Also applies to: 592-592, 607-607, 730-730, 818-818
886-892: LGTM! Execution logic with clear explanation.The require modules are executed correctly during the preload phase, mirroring the existing preload_modules pattern. The comment at lines 886-887 helpfully clarifies that while these modules are loaded as ESM (via
preload_side_module), they will be forced to be treated as CommonJS during resolution due to checks inget_known_mode_with_is_script, addressing previous review feedback.libs/resolver/cjs/mod.rs (3)
31-31: LGTM! Standard field addition.The
require_modulesfield is properly added toCjsTrackerwith correct initialization in the constructor.Also applies to: 41-41, 50-50
141-141: LGTM! Correct flag computation and propagation.The
is_from_requireflag is correctly computed by checking if the specifier is inrequire_modulesand properly propagated through the resolution chain to reach the decision point.Also applies to: 146-146, 231-231
266-268: LGTM! Critical logic for forcing require modules to CommonJS.The early return when
is_from_requireis true correctly ensures that modules loaded via--requireare always treated as CommonJS, regardless of file extension or content. The placement before other checks ensures this takes precedence, which is the intended behavior per the PR objectives.cli/tools/deploy.rs (1)
64-64: LGTM! Correct placeholder for deploy command.The empty vector is appropriate since the deploy command does not use require modules.
cli/tools/compile.rs (1)
443-445: LGTM! Ensures require modules are included in compilation.The code correctly iterates over
require_modulesand adds them tomodule_roots, mirroring the existing pattern forpreload_modules. This ensures that modules loaded via--requireare included in the compiled output's dependency graph.tests/specs/run/require_modules_multiple/require_second.js (1)
1-9: LGTM! Well-structured test module.This test file correctly validates the multiple require modules feature by:
- Using CommonJS
require()syntax- Accessing CommonJS-specific globals (
__filename,__dirname)- Exposing test globals for verification by the main test
- Providing console output for execution order verification
The test complements
require_first.jsto ensure multiple--requiremodules are loaded sequentially before the main script.tests/specs/run/require_modules_multiple/__test__.jsonc (1)
1-5: LGTM! Test specification correctly validates multiple require modules.The test spec properly exercises the new
--requireflag with multiple modules and expects successful execution.cli/tools/repl/mod.rs (1)
189-190: LGTM! Correctly extends worker creation for require_modules.The empty vector with explanatory comment is appropriate for the REPL context, which doesn't support require modules.
tests/specs/run/require_modules_multiple/main_multiple.ts (1)
1-10: LGTM! Test script correctly validates require module execution.The script properly logs global values that should be set by the required modules, enabling verification of the require_modules feature.
cli/lsp/testing/execution.rs (1)
326-327: LGTM! Correctly threads require_modules through LSP test execution.The empty vector with explanatory comment is appropriate, as LSP test execution doesn't support the require option.
cli/tools/jupyter/mod.rs (1)
105-106: LGTM! Correctly extends Jupyter worker creation for require_modules.The empty vector with explanatory comment is appropriate for the Jupyter context, which doesn't support require modules.
cli/lib/standalone/binary.rs (1)
90-90: LGTM! Field addition is correct and well-positioned.The
require_modulesfield mirrors thepreload_modulesfield structure and is appropriately positioned for serialization in standalone binaries.cli/standalone/binary.rs (2)
726-731: LGTM! require_modules computation mirrors preload_modules pattern.The transformation logic correctly resolves require module specifiers and converts them to root-directory-relative keys, matching the established pattern for preload_modules.
754-754: LGTM! Metadata initialization includes require_modules.The field is correctly included in the Metadata struct initialization, completing the propagation of require_modules into the standalone binary.
cli/lsp/config.rs (1)
1472-1472: LGTM! ResolverFactoryOptions correctly initialized with require_modules.The empty vector is appropriate for the LSP configuration path, which doesn't utilize require modules.
cli/args/mod.rs (1)
640-663: Require/preload module resolution is consistent and correct
require_modules()cleanly mirrorspreload_modules(): both resolve specifiers viaresolve_url_or_pathrelative toinitial_cwd, short‑circuit on empty input, and propagate errors. No functional issues spotted.tests/specs/run/require_modules_multiple/require_first.js (1)
1-9: Good CJS preload coverage for--requireThis fixture exercises
require("node:os"),__filename, and__dirname, and exposes markers onglobalThis, which is exactly what’s needed to verify--requiremodules run as CommonJS.libs/resolver/factory.rs (1)
661-687: Plumbingrequire_modulesinto CJS tracker looks soundAdding
require_modules: Vec<Url>toResolverFactoryOptionsand threading it intoCjsTracker::newprovides the required extra signal for CJS detection while defaulting to an empty list for non‑--requirecontexts. This is backwards‑compatible and keeps responsibilities well‑separated.Also applies to: 845-854
cli/args/flags.rs (2)
809-811: LGTM: new Flags.require fieldAdding
pub require: Vec<String>next topreloadis consistent and keeps defaults zero-cost. No concerns.
6806-6811: LGTM: parser is simple and consistent withpreload
require_arg_parsemirrorspreload_arg_parse; no issues.cli/tools/bench/mod.rs (1)
160-160: LGTM! Consistent threading of require_modules through the bench tooling.The new
require_modulesparameter is correctly propagated through the entire bench execution pipeline:
bench_specifier→bench_specifier_inner→create_custom_workerbench_specifiersappropriately clones the vector for concurrent execution- Both
run_benchmarksandrun_benchmarks_with_watchobtainrequire_modulesfromcli_optionsThe implementation follows the same pattern as
preload_modules, ensuring consistency across the codebase.Also applies to: 193-193, 308-308, 509-509, 649-649
cli/tools/test/mod.rs (2)
651-651: Appropriate lint suppression for expanded function signatures.The
#[allow(clippy::too_many_arguments)]attributes onconfigure_main_workerandtest_specifierare appropriate given that these functions now have additional parameters to supportrequire_modules. This follows the same pattern used elsewhere in the codebase for complex worker configuration functions.Also applies to: 713-713
656-656: LGTM! Correct propagation of require_modules through test execution paths.The
require_modulesparameter is properly threaded through the test execution pipeline:
test_specifier→configure_main_worker→create_custom_workertest_specifiersappropriately clones the vector for concurrent test execution- Both
run_testsandrun_tests_with_watchobtainrequire_modulesfromcli_optionsThe implementation mirrors the existing
preload_modulespattern, maintaining consistency and ensuring that required modules are available for all test execution scenarios.Also applies to: 719-719, 1178-1178, 1594-1594, 1808-1808
cli/worker.rs (1)
331-338: LGTM! Worker factory methods correctly extended with require_modules.All three worker creation methods properly accept and forward the
require_modulesparameter:
create_main_worker(lines 331, 338)create_main_worker_with_unconfigured_runtime(lines 352, 360)create_custom_worker(lines 375, 435)The parameter is consistently passed through to
LibMainWorkerFactory::create_custom_worker, ensuring that required modules are available during worker initialization. The implementation follows the established pattern used forpreload_modules.Also applies to: 352-360, 375-435
cli/tools/run/mod.rs (1)
94-94: LGTM! Complete coverage of require_modules across all run execution paths.The
require_modulesparameter has been correctly integrated into all execution modes:
run_script(lines 94, 110)run_from_stdin(lines 132, 157)run_with_watch(lines 198, 211-216)eval_command(lines 244, 271)run_eszip(lines 323, 333)Each path consistently obtains
require_modulesfromcli_options.require_modules()and forwards it to the appropriate worker creation method. This ensures uniform support for the --require flag across all runtime scenarios.Also applies to: 132-132, 198-198, 244-244, 323-323
fraidev
force-pushed
the
require
branch
2 times, most recently
from
2486454 to
efd7a58
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/specs/run/require_modules_multiple/multiple.outis excluded by!**/*.out
📒 Files selected for processing (6)
cli/factory.rs(1 hunks)cli/lib/worker.rs(5 hunks)cli/rt/run.rs(3 hunks)tests/specs/run/require_modules_multiple/main_multiple.ts(1 hunks)tests/specs/run/require_modules_multiple/require_first.js(1 hunks)tests/specs/run/require_modules_multiple/require_second.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/specs/run/require_modules_multiple/main_multiple.ts
- tests/specs/run/require_modules_multiple/require_first.js
- tests/specs/run/require_modules_multiple/require_second.js
🧰 Additional context used
🧬 Code graph analysis (2)
cli/rt/run.rs (2)
cli/args/mod.rs (1)
require_modules(653-664)libs/config/workspace/mod.rs (1)
root_dir_url(477-479)
cli/lib/worker.rs (1)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)
🔇 Additional comments (8)
cli/factory.rs (1)
1274-1274: LGTM! Error propagation is correct.The use of
?properly propagates resolution errors fromrequire_modules()through theget_or_try_init()closure, consistent with other option method calls in the codebase.cli/rt/run.rs (2)
857-868: LGTM! Correct integration with CjsTracker.The
require_modulesparameter is correctly passed toCjsTracker::new(), properly integrating the --require functionality into the standalone binary's CJS tracking.
1092-1098: LGTM! Proper threading of require_modules to worker.The
require_modulesis correctly passed to the worker factory, ensuring that required modules are available during standalone execution.cli/lib/worker.rs (5)
580-598: LGTM! Parameter threading is clean and consistent.The
require_modulesparameter is correctly added and forwarded throughcreate_main_workertocreate_custom_worker, following the same pattern aspreload_modules.
600-612: LGTM! Parameter addition is correct.The
require_modulesparameter is properly added tocreate_custom_workerand appropriately used in the worker construction at line 730.
727-733: LGTM! Worker initialization includes require_modules correctly.The
require_modulesfield is properly initialized in theLibMainWorkerconstruction, maintaining consistency with thepreload_modulespattern.
815-820: LGTM! Struct field is properly defined.The
require_modulesfield is correctly added toLibMainWorker, following the same pattern as the existingpreload_modulesfield.
880-894: LGTM! The require_modules execution is correctly implemented.The implementation properly loads
require_modulesafterpreload_modules, following the same pattern ofpreload_side_module→evaluate_module→run_event_loopwith appropriate error handling. The comment clarifies the ESM-to-CJS treatment mechanism as requested in previous review feedback.
| let require_modules = metadata | ||
| .require_modules | ||
| .iter() | ||
| .map(|key| root_dir_url.join(key).unwrap()) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
Replace .unwrap() with proper error handling.
The .unwrap() on line 855 will cause a panic if metadata.require_modules contains invalid URL components. Since metadata is deserialized from the binary and could be corrupted, this should return a helpful error message instead.
Apply this diff:
- let require_modules = metadata
- .require_modules
- .iter()
- .map(|key| root_dir_url.join(key).unwrap())
- .collect::<Vec<_>>();
+ let require_modules = metadata
+ .require_modules
+ .iter()
+ .map(|key| {
+ root_dir_url.join(key).with_context(|| {
+ format!("Failed to resolve require module: {}", key)
+ })
+ })
+ .collect::<Result<Vec<_>, _>>()?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cli/rt/run.rs around lines 852 to 856, replace the .unwrap() on
root_dir_url.join(key) with proper error handling: attempt each join and collect
into a Result<Vec<Url>, E> so failures return a descriptive error instead of
panicking; map the join error to a helpful message that includes the offending
key and the underlying join error, propagate that Result (using ? or early
return) so the caller receives a clear error when metadata.require_modules
contains an invalid URL component.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cli/rt/run.rs (1)
852-868: Avoid panicking on invalidrequire_modulesURLs deserialized from metadata
metadata.require_modulescomes from the embedded metadata; usingroot_dir_url.join(key).unwrap()will panic if any entry is malformed or no longer a valid URL segment. It’s better to fail with a structured error instead of crashing the process.You can mirror the existing
Result-based patterns in this function, for example:- let require_modules = metadata - .require_modules - .iter() - .map(|key| root_dir_url.join(key).unwrap()) - .collect::<Vec<_>>(); + let require_modules = metadata + .require_modules + .iter() + .map(|key| root_dir_url.join(key)) + .collect::<Result<Vec<_>, _>>() + .map_err(AnyError::from)?;This preserves the current behavior on success while surfacing a clear error instead of a panic when the metadata is bad.
🧹 Nitpick comments (1)
cli/lib/worker.rs (1)
723-732: LibMainWorker storingrequire_modulesis a reasonable extensionCapturing
require_modulesalongsidepreload_modulesonLibMainWorkeris the right place for this state and keeps all startup module lists together. If this grows further, you could consider a small struct to group these lists, but it’s fine as-is.Also applies to: 815-820
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/specs/run/require_modules_multiple/multiple.outis excluded by!**/*.out
📒 Files selected for processing (6)
cli/factory.rs(1 hunks)cli/lib/worker.rs(5 hunks)cli/rt/run.rs(3 hunks)tests/specs/run/require_modules_multiple/main_multiple.ts(1 hunks)tests/specs/run/require_modules_multiple/require_first.js(1 hunks)tests/specs/run/require_modules_multiple/require_second.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/specs/run/require_modules_multiple/require_second.js
- cli/factory.rs
- tests/specs/run/require_modules_multiple/require_first.js
🧰 Additional context used
🧬 Code graph analysis (2)
cli/lib/worker.rs (1)
cli/args/mod.rs (2)
require_modules(653-664)preload_modules(640-651)
cli/rt/run.rs (2)
cli/args/mod.rs (1)
require_modules(653-664)libs/config/workspace/mod.rs (1)
root_dir_url(477-479)
🔇 Additional comments (4)
tests/specs/run/require_modules_multiple/main_multiple.ts (1)
1-10: LGTM! Test structure properly validates multiple --require modules.This test file correctly validates that:
- Multiple modules can be preloaded via --require
- Preloaded modules execute before the main script
- Globals set by preloaded modules are accessible in main
- CommonJS
__filenameand__dirnamevariables work in required modulesThe logging approach is appropriate for Deno's spec test pattern, where console output is compared against expected results.
cli/rt/run.rs (1)
1086-1098: Plumbingrequire_modulesintocreate_main_workerlooks consistentPassing
require_modulesalongsidepreload_modulesintocreate_main_workerand ultimately intoLibMainWorkeraligns with how the non-standalone path handles these lists, and preserves the existing preload-before-main ordering.cli/lib/worker.rs (2)
579-612: Threadingrequire_modulesthrough LibMainWorkerFactory APIs looks correctAdding
require_modules: Vec<Url>tocreate_main_workerandcreate_custom_workerand passing it straight through to worker construction keeps the factory API coherent with the new CLI options and standalone path.
880-892: Require modules execution semantics match preload behavior and CJS comment clarifies intentExecuting each
require_module_urlviapreload_side_module+evaluate_module+run_event_loop(false)mirrors the existingpreload_modulesbehavior and ensures--requiremodules run before the main module. The comment about CJS handling viaget_known_mode_with_is_scriptis helpful for future readers and addresses the earlier confusion.
2 participants
Solution for #31154
This PR implements Node.js-compatible
--requireflag that allows preloading modules before the main script executes.Also, ensuring that any module loaded via the
--requireflag is always treated as CommonJS.Example:
deno run --require ./setup.js --require ./logger.cjs main.ts
Implementation
CjsTracker already handles CommonJS detection based on:
This change adds a fourth dimension: explicit --require modules.
The require_modules list flows through the system:
CLI flags → CliOptions.require_modules() → ResolverFactoryOptions
→ ResolverFactory → CjsTracker.new(require_modules)
When CjsTracker.is_maybe_cjs() is called, it now checks:
1. Is this specifier in require_modules? → Return true (always CJS)
2. Otherwise, use existing logic (extension, package.json, etc.)
This ensures early exit for --require modules before checking extension or package.json, guaranteeing CommonJS treatment.