feat: support runtime fallback for webcontainer #144
Merged
Conversation
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
WalkthroughThis update restructures the npm packaging and build workflows by migrating configuration from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeJS
participant napi/index.js
participant fallback.js
User->>NodeJS: require('unrs-resolver')
NodeJS->>napi/index.js: Attempt to load native binding
alt Native binding found
napi/index.js-->>NodeJS: Return native binding
else Native binding not found
napi/index.js->>napi/index.js: Check WASI fallback
alt WASI fallback found
napi/index.js-->>NodeJS: Return WASI fallback
else WASI fallback not found and SKIP_UNRS_RESOLVER_FALLBACK != '1'
napi/index.js->>fallback.js: Run postinstall check and load fallback
fallback.js-->>napi/index.js: Return fallback module
napi/index.js-->>NodeJS: Return fallback module
else All fallbacks failed
napi/index.js-->>NodeJS: Throw error with loadErrors
end
end
Assessment against linked issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 5dd2278 in 3 minutes and 12 seconds. Click for details.
- Reviewed
479lines of code in12files - Skipped
2files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:11
- Draft comment:
Updated package.json references and artifact commands – ensure switching from 'npm/package.json' to 'package.json' and publishing from 'napi/' is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .github/workflows/release-plz.yml:50
- Draft comment:
Updated jq command now uses package.json instead of npm/package.json; verify that version bump script aligns with new package structure. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. napi/index.js:366
- Draft comment:
Fallback logic is added correctly; ensure that the SKIP_UNRS_RESOLVER_FALLBACK flag prevents recursive fallback calls. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. napi/patch.mjs:7
- Draft comment:
Using string replacement to inject fallback code can be fragile; consider a more robust code transformation approach for future changes. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a legitimate code quality concern. String-based code manipulation can break if the target code changes slightly. However, this is a patch script, likely meant to be a one-time or temporary solution. The comment suggests a future improvement rather than pointing out a current bug or critical issue. The comment might be too speculative - it's suggesting potential future issues rather than current problems. Also, it doesn't provide specific alternatives for what a "more robust" approach would be. While speculative, the comment does point out a genuine code quality concern that could affect maintainability, and suggests a concrete direction for improvement. The comment should be deleted because it's more speculative and advisory rather than pointing out a clear, immediate issue that needs to be fixed. It violates the rule about not making speculative comments.
5. package.json:1
- Draft comment:
Package name and scripts have been updated (e.g. name, build commands); confirm that the removal of the separate 'npm' folder and renaming to 'unrs-resolver' is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. pnpm-workspace.yaml:10
- Draft comment:
The 'ignoreScripts: true' setting may disable postinstall scripts during development; verify that this behavior is desired. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
7. README.md:98
- Draft comment:
Typo: Consider changing the heading "Errors and Trouble Shooting" to "Errors and Troubleshooting" for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While this is a valid correction to improve consistency, our rules state we should not make purely informative comments or comments about obvious/unimportant things. A minor spelling/formatting issue in documentation is not critical enough to warrant a comment. The meaning is clear either way. The change would improve documentation quality and consistency. Technical documentation often uses "troubleshooting" as one word. While technically correct, this is too minor of an issue to warrant a PR comment. Documentation formatting nitpicks should be handled differently, like through a style guide. Delete this comment as it violates our rule about not making comments that are purely informative or unimportant. The minor spelling inconsistency does not impact functionality.
8. napi/index.js:370
- Draft comment:
Typo: It looks like the semicolon is missing afterloadErrors.push(err). Consider adding it if this is intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code appears to consistently omit semicolons throughout the file. This is likely an intentional style choice. The code will work correctly with or without the semicolon due to JavaScript's ASI. Style issues like this should typically be handled by linters/formatters rather than code review comments. Maybe there's a linting rule that requires semicolons that I'm not aware of? Maybe the codebase is inconsistent in other files? Even if there is a linting rule, it would catch this automatically. Also, the file starts with eslint-disable and prettier-ignore comments, indicating that standard style rules are intentionally not being applied. This comment about adding a semicolon should be deleted. It's a minor style issue that goes against the apparent style of the file, and the file explicitly opts out of style enforcement.
9. package.json:71
- Draft comment:
Typo: The target string "wasm32-wasip1-threads" appears to have a typo. Did you mean "wasm32-wasi-threads"? - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% I need to be very careful here. While "wasi" is more common than "wasip1", WASIP1 is actually a real thing - it's the first preview of WASI. Without deep knowledge of which target string is actually correct for this specific use case and version of the tools being used, I cannot be confident this is actually a typo. The comment assumes "wasip1" is wrong without providing evidence. What if "wasip1" is actually the correct target name for this specific toolchain? Given that this is a complex build configuration and "wasip1" is a valid term in the WASI ecosystem, we shouldn't assume it's a typo without more context. We should not keep this comment as we don't have strong evidence that it's actually incorrect - "wasip1" could be the intended target name.
Workflow ID: wflow_lGDYOYJ5CoDCgLku
You can customize 
JounQin
force-pushed
the
feat/runtime_fallback
branch
from
5dd2278 to
89ba466
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 89ba466 in 3 minutes and 52 seconds. Click for details.
- Reviewed
476lines of code in12files - Skipped
2files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:148
- Draft comment:
Ensure numeric version (14.2) is acceptable; some tools may expect a string. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that a numeric version is acceptable, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to confirm or ensure behavior. The comment does not provide a specific suggestion or point out a clear issue, so it should be removed.
2. napi/patch.mjs:7
- Draft comment:
Using a hard-coded substring for replacement is brittle; a more robust method (e.g. regex or a marker comment) could improve maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% The comment raises a valid point about maintainability - hardcoded strings for code manipulation can break if the target code changes slightly. However, this is a new file being added, and it's a relatively simple patch script. The current approach is clear and straightforward. The suggested alternatives (regex/markers) would add complexity without clear immediate benefit. The comment identifies a real potential maintenance issue. If the target code format changes even slightly (extra space, different newline), this patch would fail. While true, the current simple approach is adequate for this patch script. The suggested improvements would add complexity for a hypothetical future benefit. Delete the comment. While technically valid, the suggested change would add unnecessary complexity to a simple patch script without clear immediate benefit.
3. pnpm-workspace.yaml:10
- Draft comment:
Double-check that 'ignoreScripts: true' doesn’t unintentionally block necessary postinstall scripts. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check something, which violates the rule against asking the author to confirm or ensure behavior. It doesn't provide a specific suggestion or point out a clear issue.
4. napi/index.js:366
- Draft comment:
Add an inline comment clarifying the fallback control via SKIP_UNRS_RESOLVER_FALLBACK to avoid potential recursive loading. - Reason this comment was not posted:
Confidence changes required:40%<= threshold50%None
5. README.md:98
- Draft comment:
Typographical suggestion: Consider changing "Trouble Shooting" to "Troubleshooting" for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While this is a valid suggestion since "troubleshooting" is the more common spelling, it's an extremely minor change that doesn't affect functionality. The comment is about changed text since this section was added in this PR. However, our rules say not to make purely informative comments or comments about obvious/unimportant things. The spelling correction would improve consistency and professionalism of the documentation. Documentation quality is important. While documentation quality matters, this is too minor of an issue to warrant a PR comment. The current spelling is not incorrect, just less common. Delete this comment as it's too minor and doesn't affect functionality. We should avoid nitpicking about such small stylistic preferences.
Workflow ID: wflow_inY2BGuLLMPHs9hr
You can customize
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
napi/fallback.js (1)
42-45: 🛠️ Refactor suggestionAdd error handling for execFileSync.
The
execFileSynccall should be wrapped in try-catch to handle command failures gracefully, as mentioned in previous reviews.-execFileSync(command, args, { - cwd: __dirname, - stdio: 'inherit', -}) +try { + execFileSync(command, args, { + cwd: __dirname, + stdio: 'inherit', + }) +} catch (err) { + console.error('Failed to run napi-postinstall:', err.message) + process.exit(1) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.github/workflows/release-napi.yml(5 hunks).github/workflows/release-plz.yml(1 hunks).gitignore(1 hunks).prettierrc(1 hunks)README.md(1 hunks)napi/fallback.js(1 hunks)napi/index.js(1 hunks)napi/patch.mjs(1 hunks)npm/.gitignore(0 hunks)npm/README.md(0 hunks)npm/package.json(0 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- npm/.gitignore
- npm/package.json
- npm/README.md
🧰 Additional context used
🧠 Learnings (1)
napi/fallback.js (1)
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
🧬 Code Graph Analysis (1)
napi/index.js (2)
napi/fallback.js (1)
require(1-1)src/lib.rs (1)
require(319-334)
🪛 Biome (1.9.4)
napi/fallback.js
[error] 24-24: Illegal return statement outside of a function
(parse)
🪛 LanguageTool
README.md
[uncategorized] ~88-~88: You might be missing the article “the” here.
Context: ... absolute path to the folder containing current module. For ECMAScript modules, it is ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (13)
.prettierrc (1)
1-5: LGTM! Standard Prettier configuration.The formatting rules are appropriate and the
prettier-plugin-pkgplugin aligns well with the package restructuring changes in this PR.pnpm-workspace.yaml (1)
10-10: Good addition for fallback mechanism support.Setting
ignoreScripts: trueprevents lifecycle scripts from interfering with the new fallback logic during package installation, which is appropriate for the webcontainer runtime fallback feature..gitignore (1)
2-2: Appropriate cleanup for package restructuring.Adding
/npmto gitignore aligns with the consolidation of package configuration to the rootpackage.jsonand deprecation of the npm directory..github/workflows/release-plz.yml (1)
50-51: Correctly updated for package restructuring.The workflow now properly targets the root
package.jsoninstead of the deprecatednpm/package.json. The jq command syntax is correct for setting both version and postinstall script.napi/index.js (1)
366-372: Well-implemented fallback mechanism.The fallback logic is correctly placed in the loading sequence and includes proper error handling. The environment variable
SKIP_UNRS_RESOLVER_FALLBACKprovides a useful escape hatch for debugging or disabling the fallback when needed.README.md (3)
58-76: Excellent documentation improvements!The npm package usage examples are clear and demonstrate proper API usage with both the default export and ResolverFactory class.
82-96: Well-defined terminology section.The terminology section clearly defines key concepts like
directoryandspecifierwhich will help users understand the API.🧰 Tools
🪛 LanguageTool
[uncategorized] ~88-~88: You might be missing the article “the” here.
Context: ... absolute path to the folder containing current module. For ECMAScript modules, it is ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
98-100: Helpful error documentation.The errors and troubleshooting section provides useful context for common issues users might encounter.
napi/fallback.js (1)
8-33: Well-designed package manager detection.The package manager detection logic properly handles multiple package managers and provides clear error messaging for unsupported ones.
🧰 Tools
🪛 Biome (1.9.4)
[error] 24-24: Illegal return statement outside of a function
(parse)
.github/workflows/release-napi.yml (2)
11-11: Correct path update for package restructuring.The change from
npm/package.jsontopackage.jsonproperly reflects the package restructuring.
214-225: Proper workflow updates for new package structure.The publish steps correctly copy the root
package.jsonand use the newnapi/directory structure instead of the removednpm/directory.package.json (2)
46-73: Comprehensive napi configuration.The napi configuration includes a comprehensive list of target platforms and proper browser WASM settings for wide compatibility.
28-30: Appropriate runtime dependency.Adding
napi-postinstallas a runtime dependency is correct since it's used in the postinstall script and fallback mechanism.
JounQin
force-pushed
the
feat/runtime_fallback
branch
from
89ba466 to
3ef2ad3
Compare
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3ef2ad3 in 3 minutes and 37 seconds. Click for details.
- Reviewed
477lines of code in12files - Skipped
2files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/release-napi.yml:11
- Draft comment:
Verify switching watched file from 'npm/package.json' to 'package.json' doesn't break intended artifact generation. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to verify that a change doesn't break the intended artifact generation. This falls under the rule of not asking the author to confirm or ensure behavior is intended. It doesn't provide a specific suggestion or point out a clear issue.
2. .github/workflows/release-napi.yml:148
- Draft comment:
Ensure numeric version (14.2) is accepted by the FreeBSD build action; string might be expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the author to ensure that a numeric version is accepted by the FreeBSD build action, implying a potential issue with the type of version being used. However, it doesn't provide a specific suggestion or point out a clear issue in the code. It seems to be more of a cautionary note rather than a direct code suggestion or issue identification.
3. .github/workflows/release-napi.yml:214
- Draft comment:
Confirm that copying package.json to napi/package.json aligns with the updated npm-dir and artifact publishing flow. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm their intention regarding the copying ofpackage.json. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. The comment does not provide a specific suggestion or point out a clear issue.
4. .github/workflows/release-plz.yml:50
- Draft comment:
Update of jq to use package.json looks good – ensure version bump and postinstall script update work as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that the version bump and postinstall script update work as intended. This falls under the rule of not asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
5. napi/fallback.js:18
- Draft comment:
Consider throwing an Error if no executor is found rather than only setting exitCode and returning, to ensure failure is explicit. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% Both approaches (throwing vs exitCode) are valid ways to handle errors in Node.js. Setting exitCode and returning is actually a cleaner approach for a script that's meant to fail gracefully, as it allows the process to finish cleanly while still indicating failure. Throwing would be more abrupt and require try/catch handling. The current code already clearly indicates failure through both console.error and exitCode. The current approach might make debugging harder since there's no stack trace. Also, throwing is more idiomatic in JavaScript for error conditions. For a CLI script, clean process termination with exitCode is actually more idiomatic than throwing. The error message is clear and the failure is explicit through both logging and exit code. The current error handling approach is appropriate for this use case. The comment suggests a change that wouldn't necessarily improve the code.
6. napi/fallback.js:50
- Draft comment:
Deleting require.cache forces module reload; confirm this won’t produce unintended side effects. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that deletingrequire.cachewon't produce unintended side effects. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.
7. napi/index.js:366
- Draft comment:
The runtime fallback block is added; verify that setting SKIP_UNRS_RESOLVER_FALLBACK in fallback.js prevents recursion as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify the behavior of a specific setting, which violates the rule against asking the author to confirm or ensure behavior. It does not provide a specific suggestion or point out a clear issue.
8. napi/patch.mjs:7
- Draft comment:
Text-based patching of index.js is brittle; consider a more robust solution if index.js generation changes in the future. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid concern - string-based replacements can break if the target file changes its format or content structure. However, this seems like a simple patch script that's part of a build process. The pattern being matched is very specific. The comment doesn't suggest a concrete alternative approach. The comment might be highlighting a real maintainability concern. Future changes to index.js generation could break this patch mechanism. While the concern is valid, the comment doesn't provide actionable feedback - it just points out a potential future issue without suggesting a specific solution. This kind of speculative comment violates our rules. The comment should be deleted as it's speculative ("if index.js generation changes") and doesn't provide concrete, actionable feedback for improvement.
9. package.json:1
- Draft comment:
Name and version updates are correct; double-check that the 'files' array includes all necessary bindings (e.g., fallback.js, browser.js, index.d.ts, index.js). - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check something, which violates the rule against asking the author to confirm or double-check things. It does not provide a specific suggestion or point out a specific issue with the code.
10. pnpm-workspace.yaml:10
- Draft comment:
The global 'ignoreScripts: true' may skip postinstall scripts; ensure this does not interfere with the fallback mechanism during install in workspaces. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that a change does not interfere with a fallback mechanism, which violates the rule against asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or ask for a specific test to be written.
11. README.md:98
- Draft comment:
Typographical error: Consider changing "Errors and Trouble Shooting" to "Errors and Troubleshooting" for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While this is technically correct - "troubleshooting" is the standard spelling as one word - this is a very minor stylistic suggestion that doesn't impact functionality. The rules state not to make purely informative comments or comments about obvious/unimportant things. This feels like a very low-value comment that doesn't require action. The spelling correction would improve consistency with standard technical documentation practices. The change is simple and clear. While correct, this type of minor stylistic suggestion doesn't meet the bar for comments we want to keep - it's not pointing out a real issue that needs fixing. Delete this comment as it's too minor and doesn't identify a significant issue that needs addressing.
12. napi/index.js:370
- Draft comment:
Typo: Missing semicolon at the end of the statement. Please add a semicolon afterloadErrors.push(err). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% This is an auto-generated file, so style consistency within the file is more important than external conventions. The current style consistently omits semicolons in similar situations. Adding a semicolon here would actually make the code less consistent with the rest of the file. Also, in modern JavaScript, semicolons are often optional due to ASI (Automatic Semicolon Insertion). Maybe there's a good reason for wanting semicolons that I'm missing? Perhaps the auto-generator has a bug and should be adding semicolons? Even if semicolons might be preferred in general, changing just this one line would create inconsistency. If semicolon style needs to change, it should be handled at the generator level, not through PR comments. Delete this comment. The current style without semicolons is consistent with the rest of the auto-generated file, and changing just this one line would create inconsistency.
Workflow ID: wflow_wjteGcBdkPxNmPah
You can customize
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
napi/fallback.js (2)
24-25:⚠️ Potential issueFix syntax error: illegal return statement.
The
returnstatement on line 25 is outside of a function, which causes a syntax error. This should be replaced withprocess.exit(1)to properly terminate the script execution.Apply this diff to fix the syntax error:
- process.exitCode = 1 - return + process.exit(1)🧰 Tools
🪛 Biome (1.9.4)
[error] 25-25: Illegal return statement outside of a function
(parse)
43-46: 🛠️ Refactor suggestionAdd error handling for execFileSync.
The
execFileSynccall should be wrapped in a try-catch block to handle potential command execution failures gracefully, especially since this is a fallback mechanism that should be resilient.Apply this diff to add proper error handling:
-execFileSync(command, args, { - cwd: __dirname, - stdio: 'inherit', -}) +try { + execFileSync(command, args, { + cwd: __dirname, + stdio: 'inherit', + }) +} catch (error) { + console.error('Failed to execute fallback command:', error.message) + process.exit(1) +}
🧹 Nitpick comments (1)
README.md (1)
58-63: Consistent heading capitalization
The heading "npm package" uses lowercase, whereas other section headings use title-case (e.g., "Rust"). Consider renaming it to "npm Package" or "NPM Package" for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.github/workflows/release-napi.yml(5 hunks).github/workflows/release-plz.yml(1 hunks).gitignore(1 hunks).prettierrc(1 hunks)README.md(1 hunks)napi/fallback.js(1 hunks)napi/index.js(1 hunks)napi/patch.mjs(1 hunks)npm/.gitignore(0 hunks)npm/README.md(0 hunks)npm/package.json(0 hunks)package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
💤 Files with no reviewable changes (3)
- npm/.gitignore
- npm/README.md
- npm/package.json
✅ Files skipped from review due to trivial changes (1)
- napi/patch.mjs
🚧 Files skipped from review as they are similar to previous changes (7)
- .gitignore
- .prettierrc
- .github/workflows/release-plz.yml
- napi/index.js
- .github/workflows/release-napi.yml
- package.json
- pnpm-workspace.yaml
🧰 Additional context used
🧠 Learnings (1)
napi/fallback.js (1)
Learnt from: JounQin
PR: unrs/unrs-resolver#40
File: src/tests/resolve.rs:121-136
Timestamp: 2025-03-29T04:14:03.715Z
Learning: For tests involving npm/yarn package resolution in the unrs-resolver project, dependencies must be installed first using the appropriate package manager commands (pnpm i, yarn) in both the root directory and fixture directories like fixtures/pnp and fixtures/misc/nested-package-json.
🪛 LanguageTool
README.md
[uncategorized] ~88-~88: You might be missing the article “the” here.
Context: ... absolute path to the folder containing current module. For ECMAScript modules, it is ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Biome (1.9.4)
napi/fallback.js
[error] 25-25: Illegal return statement outside of a function
(parse)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (windows-latest)
🔇 Additional comments (1)
napi/fallback.js (1)
48-54: Fallback mechanism implementation looks solid.The environment variable setting and module cache clearing logic is well-designed for preventing repeated fallback attempts and ensuring fresh module loading. This effectively addresses the webcontainer runtime compatibility requirements.
Merged
This was referenced Jul 2, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
close #101
Important
Introduces runtime fallback for webcontainer, updates workflows, and enhances documentation and configuration.
napi/fallback.jsandnapi/index.jsfor package managers like npm, pnpm, yarn, etc..prettierrcfor code formatting preferences.release-napi.ymlandrelease-plz.ymlto referencepackage.jsoninstead ofnpm/package.json.release-napi.yml.README.mdwith usage examples and troubleshooting information.npm/README.md.package.jsonto include all necessary fields for publishing and building.pnpm-workspace.yamlto ignore scripts during installation.npm/.gitignoreandnpm/package.jsonas part of cleanup.This description was created by
for 3ef2ad3. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores