WIP: chore: refactor with deno_lib #1873
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to use deno_lib and updates to newer versions of Deno dependencies. The changes introduce a new module loader implementation and significantly restructure the bootstrap function to use the LibMainWorkerFactory instead of the previous MainWorker::bootstrap_from_options approach.
- Implements a custom module loader factory (
UtooModuleLoaderFactory) with file system-based module loading - Updates dependency versions across the board, particularly upgrading
deno_runtimefrom 0.207.0 to 0.212.0 - Refactors the bootstrap function to use
deno_libcomponents andLibMainWorkerFactory
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/runtime/src/module_loader.rs | New module implementing custom module loader with file system support and Node.js compatibility |
| crates/runtime/src/lib.rs | Major refactoring to use deno_lib components and new worker factory pattern |
| crates/runtime/fixtures/index.js | Test fixture for ES module imports with Node.js module compatibility |
| crates/runtime/fixtures/example.js | Simple Node.js example using file system operations |
| crates/runtime/Cargo.toml | Dependency version updates and addition of new required crates |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| npm_registry_permission_checker: Arc<NpmRegistryReadPermissionChecker<RealSys>>, | ||
| sys: RealSys, | ||
| in_npm_pkg_checker: DenoInNpmPackageChecker, | ||
| cjs_tracker: Arc<CjsTracker<DenoInNpmPackageChecker, RealSys>> |
There was a problem hiding this comment.
[nitpick] Missing trailing comma in function parameter list. For consistency with Rust formatting conventions, add a trailing comma after the last parameter.
| cjs_tracker: Arc<CjsTracker<DenoInNpmPackageChecker, RealSys>> | |
| cjs_tracker: Arc<CjsTracker<DenoInNpmPackageChecker, RealSys>>, |
|
|
||
| pub fn create_result(&self) -> CreateModuleLoaderResult { | ||
| let loader = Rc::new(FsModuleLoader { | ||
| shared: self.shared.clone() |
There was a problem hiding this comment.
[nitpick] Missing trailing comma in struct initialization. For consistency with Rust formatting conventions, add a trailing comma after the last field.
| shared: self.shared.clone() | |
| shared: self.shared.clone(), |
|
|
||
| pub async fn bootstrap(main_js_path: &str) -> Result<(), AnyError> { | ||
| let js_path = Path::new(main_js_path); | ||
| println!("js_path: {:?}", js_path); |
There was a problem hiding this comment.
Debug print statement should be removed or replaced with proper logging using the log crate that's already imported.
| println!("js_path: {:?}", js_path); | |
| info!("js_path: {:?}", js_path); |
| .create_main_worker(WorkerExecutionMode::Run, permissions, main_module.clone())?; | ||
|
|
||
| let exit_code = worker.run().await?; | ||
| println!("exit code: {:?}", exit_code); |
There was a problem hiding this comment.
Debug print statement should be removed or replaced with proper logging using the log crate that's already imported.
| println!("exit code: {:?}", exit_code); | |
| info!("exit code: {:?}", exit_code); |
| } | ||
| export default mod; | ||
| const __deno_export_1__ = mod; | ||
| export { __deno_export_1__ as 'module.exports' }; No newline at end of file |
There was a problem hiding this comment.
[nitpick] The export alias uses single quotes around 'module.exports' which is unconventional. Consider using double quotes or removing quotes entirely if the syntax allows it.
xusd320
force-pushed
the
next
branch
5 times, most recently
from
2ab3074 to
3345bb8
Compare
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
just readyEdits