Open
Conversation
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
WalkthroughAdds a WebAssembly resolver and JS/TS bindings: a new Rust crate (crates/biome_resolver_wasm) with wasm-bindgen exports and utilities, exposes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 5
🧹 Nitpick comments (7)
crates/biome_package/src/node_js_package/tsconfig_json.rs (1)
67-81: Consider adding rustdoc for the newly public API.Making
parsepublic is the right call for exposing this via WASM. Since this is now part of the public API surface, a brief rustdoc comment would help external consumers understand the expected inputs (absolute path to tsconfig.json, JSON content) and the return tuple semantics.📝 Suggested documentation
impl TsConfigJson { + /// Parses a `tsconfig.json` from its JSON content. + /// + /// `path` should be an absolute path to the `tsconfig.json` file. + /// Returns the parsed config and any diagnostics encountered during parsing. pub fn parse(path: &Utf8Path, json: &str) -> (Self, Vec<Error>) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_package/src/node_js_package/tsconfig_json.rs` around lines 67 - 81, Add a concise rustdoc comment to the newly public function `parse` describing its parameters and return semantics: document that `path: &Utf8Path` should be the path to the tsconfig.json (preferably absolute or the file name used for diagnostics), `json: &str` is the tsconfig JSON content, and the function returns `(Self, Vec<Error>)` where the first element is the parsed `TsConfig` (or a default on failure) and the second is collected diagnostics/errors; also mention that `initialise_paths` is invoked as a side-effect and note any behavior when deserialization fails (e.g., default value plus diagnostics).packages/@biomejs/resolver/README.md (1)
844-862: Consider documenting memory leak detection strategies.The memory management section correctly emphasises calling
.free(), but it might be helpful to mention that in development, users could track resolver instances to detect leaks. This is optional guidance, not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/README.md around lines 844 - 862, Add a short paragraph to the Memory management section advising developers how to detect resolver/memory leaks: mention tracking Resolver and MemoryFileSystem instances (e.g., increment/decrement counters when createNodeResolver() is called and when .free() is invoked), using heap snapshots or Node's --inspect/Chrome DevTools to compare retained objects, optionally using weak references or finalizers for debug builds, and tools like memwatch or heapdump to monitor growth; reference the Resolver, MemoryFileSystem, createNodeResolver(), resolver.resolve(), and .free() symbols so readers can correlate the guidance to the existing examples.packages/@biomejs/resolver/package.json (1)
73-75: Node 14 is end-of-life — consider raising the minimum version.Node.js 14 reached EOL in April 2023. While supporting older versions can be generous, it may introduce maintenance burden. Consider aligning with Node 18 LTS or later, which is the current active LTS line.
Suggested change
"engines": { - "node": ">=14.21.3" + "node": ">=18.0.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/package.json around lines 73 - 75, Update the package "engines" node version to require a maintained LTS (replace "node": ">=14.21.3" with something like "node": ">=18.0.0" or a specific current LTS such as ">=18.16.0"); after changing the "engines" field, run the project's test suite and adjust CI/workflow Node version matrix (if present) to match the new minimum so builds and publishing reflect the raised Node baseline.packages/@biomejs/wasm-resolver-web/package.json (1)
1-33: Consider adding"type": "module"to the Node.js package as well.The web package declares
"type": "module"(line 3), but the Node.js sibling package (@biomejs/wasm-resolver-nodejs) omits it. If both packages are intended to be ESM, consider aligning them for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/wasm-resolver-web/package.json around lines 1 - 33, The Node.js package `@biomejs/wasm-resolver-nodejs` omits the "type": "module" field while the web sibling declares it; add "type": "module" to the Node package's package.json (the top-level "type" key) so both packages are consistently marked as ESM, verify that "main", "exports", and any build/test tooling (e.g., require/import usage) align with ESM semantics and update any CommonJS entrypoints or build scripts if they rely on default CommonJS behavior.packages/@biomejs/resolver/tests/nodejs-fs.test.ts (1)
21-63: Consider usingafterEachfor consistent cleanup.Each test manually calls
resolver.free(). If a test fails before reachingfree(), the resource won't be released. Using Vitest'safterEachhook or a try/finally pattern would ensure cleanup even on failure.♻️ Example pattern using a shared variable and afterEach
+let resolver: Resolver | null = null; + +afterEach(() => { + resolver?.free(); + resolver = null; +}); + describe("Node.js filesystem resolver", () => { it("resolves a file that exists on disk", () => { - const resolver = Resolver.fromJsFileSystem( + resolver = Resolver.fromJsFileSystem( wasmModule, nodePathInfo, nodeReadFileUtf8, ); const result = resolver.resolve("./nodejs-fs.test.ts", __dirname); expect(result).toEqual({ path: path.join(__dirname, "nodejs-fs.test.ts") }); - - resolver.free(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/tests/nodejs-fs.test.ts around lines 21 - 63, Tests create a Resolver via Resolver.fromJsFileSystem and call resolver.free() manually inside each it block, which can leak resources if a test fails; change the tests to assign the created Resolver to a shared variable (e.g., resolver) and add a Vitest afterEach hook that checks and calls resolver.free() (or use try/finally around each test) so the resolver is always freed; update references in these specs (the resolver variable used in "resolves a file that exists on disk", "resolves a relative sibling file on disk", and "returns an error for a module that does not exist") to rely on the shared cleanup instead of calling resolver.free() inside each test.packages/@biomejs/resolver/tests/memory-fs.test.ts (1)
27-110: Usetry/finallyfor WASM cleanup in tests.If an assertion fails mid-test,
resolver.free()/fs.free()won’t run. A tinytry/finallywrapper keeps tests leak-safe and less flaky.♻️ Suggested pattern
it("resolves a relative path to a JS file", () => { const fs = makeMemFs(); - fs.insertFile("/project/src/index.js", ""); - fs.insertFile( - "/project/package.json", - JSON.stringify({ name: "project", version: "1.0.0" }), - ); - - const resolver = Resolver.fromMemoryFileSystem(wasmModule, fs); - - const result = resolver.resolve("./index.js", "/project/src"); - expect(result).toEqual({ path: "/project/src/index.js" }); - - resolver.free(); - fs.free(); + let resolver: Resolver | undefined; + try { + fs.insertFile("/project/src/index.js", ""); + fs.insertFile( + "/project/package.json", + JSON.stringify({ name: "project", version: "1.0.0" }), + ); + resolver = Resolver.fromMemoryFileSystem(wasmModule, fs); + const result = resolver.resolve("./index.js", "/project/src"); + expect(result).toEqual({ path: "/project/src/index.js" }); + } finally { + resolver?.free(); + fs.free(); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/tests/memory-fs.test.ts around lines 27 - 110, Wrap each test's resource allocation (makeMemFs(), Resolver.fromMemoryFileSystem(...)) in a try/finally so resolver.free() and fs.free() run even if an assertion throws; for example, after creating fs and resolver in tests like the ones using Resolver.fromMemoryFileSystem, call resolver.free() and fs.free() inside the finally block (ensure you handle the case where resolver or fs might be undefined if construction failed) to prevent WASM/resource leaks.crates/biome_resolver_wasm/src/lib.rs (1)
34-40: Use Rust doctest-format examples in rustdoc blocks.This fenced TypeScript example is useful, but it won’t run as a Rust doctest. Consider moving TS examples to package docs and keeping rustdoc examples runnable.
As per coding guidelines, "Use doc tests (doctest) format with code blocks in rustdoc comments; ensure assertions pass in tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_resolver_wasm/src/lib.rs` around lines 34 - 40, The doc comment contains a TypeScript fenced example that won’t run as a Rust doctest; either move the TS example to external/package docs and replace this rustdoc block with a runnable Rust doctest, or convert it in-place to a ```rust doctest that demonstrates calling the resolver.resolve method and matching on ResolveErrorKind (e.g., using resolver.resolve(...), assert_eq!/matches! on ResolveErrorKind::ModuleNotFound) so the block compiles and asserts as a doctest; update the docblock around resolver.resolve and ResolveErrorKind accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_resolver_wasm/Cargo.toml`:
- Around line 31-33: Cargo.toml pins wasm-bindgen to "=0.2.106" but the justfile
still installs wasm-bindgen-cli v0.2.105; open the justfile and update both
occurrences of "wasm-bindgen-cli --version 0.2.105" to "wasm-bindgen-cli
--version 0.2.106" so the CLI install matches the crate pin (ensure both
instances mentioned in the review are changed).
In `@crates/biome_resolver_wasm/src/lib.rs`:
- Around line 232-236: The loop silently ignores malformed package.json because
parse_package_json failures are treated like "not found"; change the handling so
parse errors are surfaced: make parse_package_json return a Result<Manifest,
ParseError> (or add a parse variant like parse_package_json_strict) and in the
search block that calls self.read_file_utf8(&candidate) check the Result — if
parsing returns Err, return Err with that parse error instead of continuing to
walk upward; keep the existing Ok((dir, manifest)) return when parsing succeeds.
Ensure the code path references the existing symbols self.read_file_utf8 and
parse_package_json (or the new parse function) so callers see manifest parse
failures instead of silently skipping them.
- Around line 518-521: parse_tsconfig_json currently swallows parse errors by
ignoring the `_errors` returned from TsConfigJson::parse, causing it to always
return Some(...) and preventing the caller's
`.ok_or(ResolveError::ErrorLoadingManifest)` from ever firing; change
parse_tsconfig_json to check the returned `_errors` from
`TsConfigJson::parse(path, content)` and return `None` when there are parse
errors (i.e., `_errors` is non-empty), otherwise return `Some(tsconfig)` so the
caller (using `.ok_or(ResolveError::ErrorLoadingManifest)`) can detect and
handle malformed tsconfig.json.
In `@packages/`@biomejs/resolver/src/common.ts:
- Around line 248-249: Update the stale comment API names to match the current
exported functions: replace mentions of insert() and Resolver.with* with
insertFile / insertBytes and fromJsFileSystem / fromMemoryFileSystem
respectively; ensure any other occurrences in the same file (e.g., around the
second noted block) are updated so the comment refers to insertFile/insertBytes
and the factory helpers fromJsFileSystem/fromMemoryFileSystem on the Resolver
export.
- Around line 349-350: Update the JSDoc that currently reads "Returns `{ path:
string }` on success or `{ error: string }` on failure" to include the
structured failure shape that callers receive (e.g. "or `{ error: string;
errorKind: string }`"), and if there is an existing enum/union used by the
implementation (for example ErrorKind, ResolveErrorKind, or similar), reference
those exact symbol names and list the possible values so callers can branch on
errorKind.
---
Nitpick comments:
In `@crates/biome_package/src/node_js_package/tsconfig_json.rs`:
- Around line 67-81: Add a concise rustdoc comment to the newly public function
`parse` describing its parameters and return semantics: document that `path:
&Utf8Path` should be the path to the tsconfig.json (preferably absolute or the
file name used for diagnostics), `json: &str` is the tsconfig JSON content, and
the function returns `(Self, Vec<Error>)` where the first element is the parsed
`TsConfig` (or a default on failure) and the second is collected
diagnostics/errors; also mention that `initialise_paths` is invoked as a
side-effect and note any behavior when deserialization fails (e.g., default
value plus diagnostics).
In `@crates/biome_resolver_wasm/src/lib.rs`:
- Around line 34-40: The doc comment contains a TypeScript fenced example that
won’t run as a Rust doctest; either move the TS example to external/package docs
and replace this rustdoc block with a runnable Rust doctest, or convert it
in-place to a ```rust doctest that demonstrates calling the resolver.resolve
method and matching on ResolveErrorKind (e.g., using resolver.resolve(...),
assert_eq!/matches! on ResolveErrorKind::ModuleNotFound) so the block compiles
and asserts as a doctest; update the docblock around resolver.resolve and
ResolveErrorKind accordingly.
In `@packages/`@biomejs/resolver/package.json:
- Around line 73-75: Update the package "engines" node version to require a
maintained LTS (replace "node": ">=14.21.3" with something like "node":
">=18.0.0" or a specific current LTS such as ">=18.16.0"); after changing the
"engines" field, run the project's test suite and adjust CI/workflow Node
version matrix (if present) to match the new minimum so builds and publishing
reflect the raised Node baseline.
In `@packages/`@biomejs/resolver/README.md:
- Around line 844-862: Add a short paragraph to the Memory management section
advising developers how to detect resolver/memory leaks: mention tracking
Resolver and MemoryFileSystem instances (e.g., increment/decrement counters when
createNodeResolver() is called and when .free() is invoked), using heap
snapshots or Node's --inspect/Chrome DevTools to compare retained objects,
optionally using weak references or finalizers for debug builds, and tools like
memwatch or heapdump to monitor growth; reference the Resolver,
MemoryFileSystem, createNodeResolver(), resolver.resolve(), and .free() symbols
so readers can correlate the guidance to the existing examples.
In `@packages/`@biomejs/resolver/tests/memory-fs.test.ts:
- Around line 27-110: Wrap each test's resource allocation (makeMemFs(),
Resolver.fromMemoryFileSystem(...)) in a try/finally so resolver.free() and
fs.free() run even if an assertion throws; for example, after creating fs and
resolver in tests like the ones using Resolver.fromMemoryFileSystem, call
resolver.free() and fs.free() inside the finally block (ensure you handle the
case where resolver or fs might be undefined if construction failed) to prevent
WASM/resource leaks.
In `@packages/`@biomejs/resolver/tests/nodejs-fs.test.ts:
- Around line 21-63: Tests create a Resolver via Resolver.fromJsFileSystem and
call resolver.free() manually inside each it block, which can leak resources if
a test fails; change the tests to assign the created Resolver to a shared
variable (e.g., resolver) and add a Vitest afterEach hook that checks and calls
resolver.free() (or use try/finally around each test) so the resolver is always
freed; update references in these specs (the resolver variable used in "resolves
a file that exists on disk", "resolves a relative sibling file on disk", and
"returns an error for a module that does not exist") to rely on the shared
cleanup instead of calling resolver.free() inside each test.
In `@packages/`@biomejs/wasm-resolver-web/package.json:
- Around line 1-33: The Node.js package `@biomejs/wasm-resolver-nodejs` omits the
"type": "module" field while the web sibling declares it; add "type": "module"
to the Node package's package.json (the top-level "type" key) so both packages
are consistently marked as ESM, verify that "main", "exports", and any
build/test tooling (e.g., require/import usage) align with ESM semantics and
update any CommonJS entrypoints or build scripts if they rely on default
CommonJS behavior.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockand included by**pnpm-lock.yamlis excluded by!**/pnpm-lock.yamland included by**
📒 Files selected for processing (21)
Cargo.tomlcrates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_resolver_wasm/Cargo.tomlcrates/biome_resolver_wasm/src/lib.rscrates/biome_resolver_wasm/src/utils.rsjustfilepackages/@biomejs/resolver/README.mdpackages/@biomejs/resolver/package.jsonpackages/@biomejs/resolver/src/common.tspackages/@biomejs/resolver/src/index.tspackages/@biomejs/resolver/src/nodejs.tspackages/@biomejs/resolver/src/nodejsFileSystem.tspackages/@biomejs/resolver/src/web.tspackages/@biomejs/resolver/tests/memory-fs.test.tspackages/@biomejs/resolver/tests/nodejs-fs.test.tspackages/@biomejs/resolver/tsconfig.jsonpackages/@biomejs/resolver/vitest.config.tspackages/@biomejs/wasm-resolver-nodejs/.gitignorepackages/@biomejs/wasm-resolver-nodejs/package.jsonpackages/@biomejs/wasm-resolver-web/.gitignorepackages/@biomejs/wasm-resolver-web/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_resolver_wasm/src/lib.rs`:
- Around line 322-330: Doc examples for the fields extensions and
extension_aliases use leading dots which conflict with the no-dot contract;
update the comment text for the fields extensions: Vec<String> and
extension_aliases: Vec<WasmExtensionAlias> (and any inline example for
WasmExtensionAlias) to show extensions without leading periods (e.g. "js", "ts",
"json" and alias entries like { extension: "js", aliases: ["ts","js"] }) so they
match the API contract.
In `@packages/`@biomejs/resolver/README.md:
- Around line 56-57: Replace the awkward phrase "Head the [relative section]" in
the README sentence with clearer wording — e.g., change it to "Head to the
relevant section" (so the line reads: "Head to the [relative
section](`#using-the-web-distribution`) if you wish to know how to use the web
distribution.").
- Around line 670-680: The README currently shows a static import for
createMemoryFileSystem and createWebResolver but the text says the web
distribution must be loaded with dynamic import(); update the example to
demonstrate using await import() to load "@biomejs/resolver/web" before calling
createMemoryFileSystem and createWebResolver, ensuring the snippet awaits the
dynamic import so the WASM is fetched/compiled before any resolver APIs are
used; reference the symbols createMemoryFileSystem and createWebResolver in the
updated example and keep the explanatory sentence about awaiting the WASM.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/biome_package/src/node_js_package/tsconfig_json.rscrates/biome_resolver_wasm/src/lib.rsjustfilepackages/@biomejs/resolver/README.mdpackages/@biomejs/resolver/src/common.tspackages/@biomejs/resolver/tests/memory-fs.test.tspackages/@biomejs/resolver/tests/nodejs-fs.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- justfile
- crates/biome_package/src/node_js_package/tsconfig_json.rs
- packages/@biomejs/resolver/tests/memory-fs.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/@biomejs/resolver/README.md (1)
668-722: Consider consolidating the two web-distribution walkthroughs.Both sections are good, but they overlap a lot. Keeping one canonical deep-dive and linking to it from the other will reduce doc drift over time.
Also applies to: 878-958
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/README.md around lines 668 - 722, Consolidate the two overlapping web-distribution walkthroughs into a single canonical example: keep the detailed example that demonstrates createMemoryFileSystem, fs.insertFile, createWebResolver, resolver.resolve, resolver.free and fs.free as the primary deep-dive, remove the duplicate section, and replace the removed section with a short pointer/link to the canonical walkthrough (and vice versa if you prefer the other block as canonical); ensure the canonical example includes the same options (extensions, defaultFiles, conditionNames) and the same sample resolve calls so callers can find the full example from either location.packages/@biomejs/resolver/tests/nodejs-fs.test.ts (1)
22-26: Small DRY win: extract resolver construction into a helper.Same constructor block appears three times; a tiny helper will keep future test edits tidier.
♻️ Suggested refactor
+function createTestResolver() { + return Resolver.fromJsFileSystem( + wasmModule, + nodePathInfo, + nodeReadFileUtf8, + ); +} + describe("Node.js filesystem resolver", () => { it("resolves a file that exists on disk", () => { - const resolver = Resolver.fromJsFileSystem( - wasmModule, - nodePathInfo, - nodeReadFileUtf8, - ); + const resolver = createTestResolver(); try { const result = resolver.resolve("./nodejs-fs.test.ts", __dirname); expect(result).toEqual({ path: path.join(__dirname, "nodejs-fs.test.ts"), }); } finally { resolver.free(); } });Also applies to: 38-42, 54-58
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/tests/nodejs-fs.test.ts around lines 22 - 26, The test repeats the Resolver.fromJsFileSystem(wasmModule, nodePathInfo, nodeReadFileUtf8) construction in multiple places; extract that into a small helper function (e.g., makeResolver or createTestResolver) inside the test file and replace the three inline instances with calls to that helper, referencing Resolver.fromJsFileSystem, wasmModule, nodePathInfo, and nodeReadFileUtf8 so the helper returns the configured resolver for reuse.crates/biome_resolver_wasm/src/lib.rs (1)
214-216: Validate symlink targets are absolute at the JS boundary.At Line 215, any string is accepted as
canonicalized_target. Guarding for absolute paths here makes host callback mistakes fail fast and avoids confusing downstream resolution.🛡️ Suggested guard
- return Ok(PathInfo::Symlink { - canonicalized_target: Utf8PathBuf::from(target), - }); + let target = Utf8PathBuf::from(target); + if !target.is_absolute() { + return Err(ResolveError::BrokenSymlink); + } + return Ok(PathInfo::Symlink { + canonicalized_target: target, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_resolver_wasm/src/lib.rs` around lines 214 - 216, The code currently accepts any string into PathInfo::Symlink::canonicalized_target (Utf8PathBuf::from(target)); add a guard that validates the target is an absolute path before constructing the Utf8PathBuf. In the branch that returns PathInfo::Symlink, call Utf8Path::new(&target).is_absolute() (or Utf8PathBuf::is_absolute()) and if it is not absolute return an Err (using the crate's existing error type/variant) with a clear message including the offending target; only construct Utf8PathBuf::from(target) and return Ok(PathInfo::Symlink { canonicalized_target: ... }) when the check passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_resolver_wasm/src/lib.rs`:
- Around line 465-468: The doc comment for the resolver's return shape is
incomplete: update the comment in lib.rs above the resolve() function to
document that failures return both `error: string` and `errorKind: string`
(e.g., `{ error: string, errorKind: string }`), briefly describing each field so
callers know to expect the error kind in addition to the message.
In `@packages/`@biomejs/resolver/README.md:
- Around line 726-731: The README's sentence claiming "`resolver.resolve()`
never throws" is too absolute; update the wording to clarify that
resolver.resolve() does not throw for resolution failures — it returns a result
object where a failed result contains `error` and `errorKind` (a
`ResolveErrorKind`), and a successful result contains `path` — but do not imply
that all misuse or unexpected runtime errors cannot throw (e.g., invalid
arguments or environment errors may still raise exceptions). Locate the
description around `resolver.resolve()`, `ResolveErrorKind`, `path`, and `error`
and rephrase to scope the "no throw" guarantee specifically to normal resolution
outcomes.
---
Nitpick comments:
In `@crates/biome_resolver_wasm/src/lib.rs`:
- Around line 214-216: The code currently accepts any string into
PathInfo::Symlink::canonicalized_target (Utf8PathBuf::from(target)); add a guard
that validates the target is an absolute path before constructing the
Utf8PathBuf. In the branch that returns PathInfo::Symlink, call
Utf8Path::new(&target).is_absolute() (or Utf8PathBuf::is_absolute()) and if it
is not absolute return an Err (using the crate's existing error type/variant)
with a clear message including the offending target; only construct
Utf8PathBuf::from(target) and return Ok(PathInfo::Symlink {
canonicalized_target: ... }) when the check passes.
In `@packages/`@biomejs/resolver/README.md:
- Around line 668-722: Consolidate the two overlapping web-distribution
walkthroughs into a single canonical example: keep the detailed example that
demonstrates createMemoryFileSystem, fs.insertFile, createWebResolver,
resolver.resolve, resolver.free and fs.free as the primary deep-dive, remove the
duplicate section, and replace the removed section with a short pointer/link to
the canonical walkthrough (and vice versa if you prefer the other block as
canonical); ensure the canonical example includes the same options (extensions,
defaultFiles, conditionNames) and the same sample resolve calls so callers can
find the full example from either location.
In `@packages/`@biomejs/resolver/tests/nodejs-fs.test.ts:
- Around line 22-26: The test repeats the Resolver.fromJsFileSystem(wasmModule,
nodePathInfo, nodeReadFileUtf8) construction in multiple places; extract that
into a small helper function (e.g., makeResolver or createTestResolver) inside
the test file and replace the three inline instances with calls to that helper,
referencing Resolver.fromJsFileSystem, wasmModule, nodePathInfo, and
nodeReadFileUtf8 so the helper returns the configured resolver for reuse.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/biome_resolver_wasm/src/lib.rspackages/@biomejs/resolver/README.mdpackages/@biomejs/resolver/tests/memory-fs.test.tspackages/@biomejs/resolver/tests/nodejs-fs.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@biomejs/resolver/tests/memory-fs.test.ts
Comment on lines
+465
to
+468
| /// Returns a plain JavaScript object: | ||
| /// - On success: `{ path: string }` — the resolved absolute path. | ||
| /// - On failure: `{ error: string }` — a description of why resolution | ||
| /// failed. |
Contributor
There was a problem hiding this comment.
Failure return-shape docs are missing errorKind.
At Line 467, docs say failure returns only { error: string }, but resolve() also sets errorKind. Please document both fields to avoid copy-paste integration bugs.
✏️ Doc tweak
- /// - On failure: `{ error: string }` — a description of why resolution
- /// failed.
+ /// - On failure: `{ error: string, errorKind: ResolveErrorKind }` —
+ /// a description plus a machine-readable failure kind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_resolver_wasm/src/lib.rs` around lines 465 - 468, The doc
comment for the resolver's return shape is incomplete: update the comment in
lib.rs above the resolve() function to document that failures return both
`error: string` and `errorKind: string` (e.g., `{ error: string, errorKind:
string }`), briefly describing each field so callers know to expect the error
kind in addition to the message.
Comment on lines
+726
to
+731
| `resolver.resolve()` never throws. All failures are returned as a value. A | ||
| failed result carries two fields: `error`, a human-readable string suitable for | ||
| logging, and `errorKind`, a `ResolveErrorKind` enum value for programmatic | ||
| branching. A successful result carries only `path`. Exactly one of `path` or | ||
| `error` is always present. | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid an absolute “never throws” claim.
On Line 726, “never throws” is a bit too strong. It’s safer to scope this to resolution failures so readers don’t assume API misuse can’t throw.
✏️ Suggested wording tweak
-`resolver.resolve()` never throws. All failures are returned as a value. A
+`resolver.resolve()` does not throw for resolution failures; failures are returned as values. A
failed result carries two fields: `error`, a human-readable string suitable for
logging, and `errorKind`, a `ResolveErrorKind` enum value for programmatic
branching. A successful result carries only `path`. Exactly one of `path` or
`error` is always present.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| `resolver.resolve()` never throws. All failures are returned as a value. A | |
| failed result carries two fields: `error`, a human-readable string suitable for | |
| logging, and `errorKind`, a `ResolveErrorKind` enum value for programmatic | |
| branching. A successful result carries only `path`. Exactly one of `path` or | |
| `error` is always present. | |
| `resolver.resolve()` does not throw for resolution failures; failures are returned as values. A | |
| failed result carries two fields: `error`, a human-readable string suitable for | |
| logging, and `errorKind`, a `ResolveErrorKind` enum value for programmatic | |
| branching. A successful result carries only `path`. Exactly one of `path` or | |
| `error` is always present. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/`@biomejs/resolver/README.md around lines 726 - 731, The README's
sentence claiming "`resolver.resolve()` never throws" is too absolute; update
the wording to clarify that resolver.resolve() does not throw for resolution
failures — it returns a result object where a failed result contains `error` and
`errorKind` (a `ResolveErrorKind`), and a successful result contains `path` —
but do not imply that all misuse or unexpected runtime errors cannot throw
(e.g., invalid arguments or environment errors may still raise exceptions).
Locate the description around `resolver.resolve()`, `ResolveErrorKind`, `path`,
and `error` and rephrase to scope the "no throw" guarantee specifically to
normal resolution outcomes.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/@biomejs/resolver/README.md (1)
729-733:⚠️ Potential issue | 🟡 MinorScope the no-throw guarantee to resolution outcomes only.
This still reads as an absolute runtime guarantee, which is too strong. Please narrow it to “does not throw for normal resolution failures” and keep room for misuse/runtime exceptions (e.g. invalid args or freed instances). Same issue propagates into the “never need try/catch” sentence later.
✏️ Suggested wording tweak
-`resolver.resolve()` never throws. All failures are returned as a value. A +`resolver.resolve()` does not throw for resolution failures; failures are returned as values. A failed result carries two fields: `error`, a human-readable string suitable for logging, and `errorKind`, a `ResolveErrorKind` enum value for programmatic branching. A successful result carries only `path`. Exactly one of `path` or `error` is always present. @@ -This design means you never need a `try`/`catch` around `resolve()` calls. All -exceptional conditions, including malformed manifests and broken symlinks, are -surfaced as values rather than thrown exceptions. +In normal resolution flows, you typically do not need a `try`/`catch` around +`resolve()` calls, because resolution failures are surfaced as values rather than +thrown exceptions.Also applies to: 766-768
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/`@biomejs/resolver/README.md around lines 729 - 733, Update the README wording for resolver.resolve() to narrowly state that it "does not throw for normal resolution failures" (i.e., failure results are returned as values with error and errorKind) while clarifying that invalid usage or unexpected runtime issues (invalid arguments, freed instances, or other exceptions) may still throw; apply the same scoped language change to the later sentence that currently says "never need try/catch" so it instead advises that callers typically don’t need try/catch for resolution outcomes but should still handle or allow exceptions for misuse/runtime errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/`@biomejs/resolver/README.md:
- Around line 729-733: Update the README wording for resolver.resolve() to
narrowly state that it "does not throw for normal resolution failures" (i.e.,
failure results are returned as values with error and errorKind) while
clarifying that invalid usage or unexpected runtime issues (invalid arguments,
freed instances, or other exceptions) may still throw; apply the same scoped
language change to the later sentence that currently says "never need try/catch"
so it instead advises that callers typically don’t need try/catch for resolution
outcomes but should still handle or allow exceptions for misuse/runtime errors.
arendjr
approved these changes
Mar 4, 2026
Contributor
arendjr
left a comment
arendjr
left a comment
There was a problem hiding this comment.
Nice work! Only left some suggestions.
Comment on lines
+72
to
+76
| /// relative paths inside the config (e.g. `baseUrl`, `paths`). Must be | ||
| /// an absolute path; passing a relative path will cause a panic inside | ||
| /// [`Self::initialise_paths`]. | ||
| /// * `json` — the raw JSON (or JSONC) content of the file. | ||
| /// |
Contributor
There was a problem hiding this comment.
Suggested change
| /// relative paths inside the config (e.g. `baseUrl`, `paths`). Must be | |
| /// an absolute path; passing a relative path will cause a panic inside | |
| /// [`Self::initialise_paths`]. | |
| /// * `json` — the raw JSON (or JSONC) content of the file. | |
| /// | |
| /// relative paths inside the config (e.g. `baseUrl`, `paths`). Must be | |
| /// an absolute path. | |
| /// * `json` — the raw JSON (or JSONC) content of the file. | |
| /// | |
| /// # Panics | |
| /// | |
| /// If the given `path` is not an absolute path. |
| @@ -0,0 +1,373 @@ | |||
| /** | |||
Contributor
There was a problem hiding this comment.
This file is handwritten, right?
| * | ||
| * Extensions must be provided **without a leading dot**. | ||
| * | ||
| * Example: `[{ extension: "js", aliases: ["ts", "js"] }]` |
Contributor
There was a problem hiding this comment.
This example may be slightly misleading? I don't think "js" should be provided as an alias, since it's already the extension itself.
Comment on lines
+67
to
+68
| * When `true`, resolve TypeScript declaration files (`.d.ts`) instead of | ||
| * source files. |
Contributor
There was a problem hiding this comment.
Suggested change
| * When `true`, resolve TypeScript declaration files (`.d.ts`) instead of | |
| * source files. | |
| * When `true`, resolve TypeScript declaration files (`.d.ts`) in addition to source files. | |
| * If no declaration file is found, the resolver may still resolve to a source file. |
| /** | ||
| * Which WASM distribution to load. | ||
| */ | ||
| export enum Distribution { |
Contributor
There was a problem hiding this comment.
What is this enum used for? I don't see it being used elsewhere in this PR either...
Member
Author
There was a problem hiding this comment.
Probably a copy-paste from how we do stuff in js-api. Probably not needed
Comment on lines
+22
to
+24
| "sideEffects": [ | ||
| "./snippets/*" | ||
| ], |
Contributor
There was a problem hiding this comment.
Copy-paste leftover?
Suggested change
| "sideEffects": [ | |
| "./snippets/*" | |
| ], |
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.
Summary
Note
PR implemented with the help of AI
This PR adds a new
@biomejs/resolverpackage, which exposes our internalbiome_resolverto the npm world.The PR doesn't set up the workflow yet (we need to create an empty package manually in the npm first).
Let me know what do you think of the documentation and the APIs!
Test Plan
Craeted new tests
Docs
The docs were added to the README.md