Feat/bundler#14
Conversation
feat: supported `require` CJS
There was a problem hiding this comment.
Pull request overview
This pull request introduces a major architectural improvement to Xmas.JS by adding a Virtual System Layer (vsys) and bundler functionality. The vsys abstraction provides pluggable implementations for filesystem operations, network access, and module loading, enabling sandboxed execution, custom backends, and fine-grained permission control.
Key Changes:
- New
vsyscrate with virtual tables for filesystem, module loading, and permissions - New
bundlercrate powered by Rolldown for fast TypeScript/JavaScript bundling - Complete rewrite of filesystem module to use vsys abstraction
- Refactored permissions system to delegate to vsys
- Updated module resolver/loader with improved logging
- Integration of vsys and bundler into REPL
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| vsys/Cargo.toml | New crate configuration with dependencies for JSON parsing, UUID generation, and platform-specific libc |
| vsys/src/lib.rs | Core vsys structure with builder pattern, default and sandboxed constructors |
| vsys/src/error.rs | Comprehensive error types with C ABI compatibility |
| vsys/src/fs.rs | Complete filesystem vtable with sync operations, file handles, and platform abstractions |
| vsys/src/permissions.rs | Black/whitelist permission system with path and host checking |
| vsys/src/module_loader.rs | Module resolution and loading vtable supporting ESM, CJS, JSON, and built-ins |
| bundler/Cargo.toml | New bundler crate using Rolldown from Git repository |
| bundler/src/lib.rs | Bundler configuration and implementation with multiple output formats |
| modules/Cargo.toml | Added vsys dependency and updated oxc versions |
| modules/src/lib.rs | Changed init signature to accept Arc instead of Permissions |
| modules/src/permissions.rs | Refactored to wrap and delegate to vsys, added helper functions |
| modules/src/fs.rs | Complete rewrite using vsys FsVTable instead of direct std::fs calls |
| modules/src/fetch/security.rs | Simplified to use vsys permission checking |
| modules/src/module/**/*.rs | Updated logging levels from trace to info/debug with emoji indicators |
| repl/Cargo.toml | Added vsys and bundler dependencies |
| repl/src/lib.rs | Integrated vsys builder and bundler command in REPL |
| README.md | Added comprehensive vsys documentation with architecture diagrams |
| TODO.md | Added vsys implementation checklist |
| Cargo.toml | Added vsys and bundler to workspace members |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # source | ||
| oxc = { version = "0.105.0", optional = true, features = [ | ||
| oxc = { version = "^0.103.0", optional = true, features = [ |
There was a problem hiding this comment.
The version constraint "^0.103.0" is inconsistent with the existing oxc dependency patterns. Consider whether this downgrade from "0.105.0" is intentional and if both version constraints should align.
| oxc = { version = "^0.103.0", optional = true, features = [ | |
| oxc = { version = "^0.105.0", optional = true, features = [ |
| pub fn check_path(&self, path: &Path) -> bool { | ||
| let canonical_path = match path.canonicalize() { | ||
| Ok(p) => p, | ||
| Err(_) => return false, | ||
| }; |
There was a problem hiding this comment.
The check_path method returns false if canonicalization fails. This could silently allow or deny access for paths with issues like symlink loops or non-existent parent directories. Consider returning a Result instead or documenting this behavior clearly, as it could lead to unexpected permission bypasses or denials.
| debug!("Try resolve '{}' from '{}'", name, base); | ||
|
|
||
| require_resolve(ctx, name, base, None, true).map(|name| name.into_owned()) | ||
| require_resolve(ctx, name, base, true).map(|name| name.into_owned()) |
There was a problem hiding this comment.
The require_resolve function signature was changed to remove the hooked_fn parameter, but this removes functionality that may be needed. Verify that all callers have been updated and that removing this hook mechanism doesn't break existing functionality.
| println!("❄️\t{} - Package manager commands", "/pm".cyan().bold()); | ||
| println!("❄️\t{} - Cross platform shell commands", "/$".cyan().bold()); | ||
| println!("❄️\t{} - Bundle JavaScript/TypeScript files", "/bun".cyan().bold()); | ||
|
|
There was a problem hiding this comment.
There's a blank line followed by an extra closing brace indentation inconsistency. Remove the extra blank line to maintain consistent formatting.
| pub async fn read<'js>(&mut self, ctx: Ctx<'js>, size: Opt<usize>) -> Result<Value<'js>> { | ||
| let handle = self | ||
| .handle | ||
| .as_mut() | ||
| .ok_or_else(|| Exception::throw_message(&ctx, "File handle is closed"))?; | ||
|
|
||
| let size = size.0.unwrap_or(4096); | ||
| let mut buf = vec![0u8; size]; | ||
|
|
||
| let n = handle | ||
| .read(&mut buf) | ||
| .map_err(|e| Exception::throw_message(&ctx, &e.to_string()))?; | ||
|
|
||
| buf.truncate(n); | ||
| Buffer(buf).into_js(&ctx) | ||
| } |
There was a problem hiding this comment.
The FileHandle read method doesn't handle the case where the file handle could be accessed concurrently from multiple async tasks. Since the handle is &mut self and the inner handle is mutable, consider documenting thread-safety guarantees or adding synchronization if concurrent access is expected.
| version = "0.1.0" | ||
| edition = "2021" | ||
|
|
||
| [dependencies] |
There was a problem hiding this comment.
The bundler depends on a Git repository with a specific tag. This creates a potential supply chain risk and makes builds non-reproducible if the repository is unavailable. Consider using a published crate version from crates.io instead, or if the crate is not published yet, document this as a temporary dependency.
| [dependencies] | |
| [dependencies] | |
| # Temporary Git dependency: rolldown is currently consumed from the upstream repository rather than crates.io. | |
| # The dependency is pinned to tag v1.0.0-beta.57 for reproducible builds. Replace this with a crates.io | |
| # version once an appropriate release is available. |
| oxc_resolver = "=11.16.0" | ||
| thiserror = "*" |
There was a problem hiding this comment.
The oxc_resolver version is pinned with "=" which is overly restrictive and could cause dependency conflicts. Consider using a caret requirement like "^11.16.0" or tilde requirement "~11.16.0" to allow for compatible patches. Also, the thiserror version uses "*" which is too permissive and could break builds. Specify an explicit version constraint.
| oxc_resolver = "=11.16.0" | |
| thiserror = "*" | |
| oxc_resolver = "^11.16.0" | |
| thiserror = "^1.0.0" |
No description provided.