feat: merge from upstream oxc-project/oxc-resolver - 5th
#94
Conversation
Node supports `package.json` with UTF-8 BOM and TypeScript supports `tsconfig.json` with UTF-8 BOM. An error was happening for these cases.
WalkthroughThis update introduces support for handling files with a Byte Order Mark (BOM) in both package.json and tsconfig.json files. Internal parsing logic is updated to strip the BOM before deserialization. New test fixtures and test cases are added to verify correct resolution behavior when BOMs are present. Additionally, the resolver's internal control flow is optimized by passing parsed package specifier components directly to relevant methods, reducing redundant parsing. The README is updated to reference an additional synchronized pull request. Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant Resolver
participant Parser
Test->>Resolver: resolve(specifier, base_dir)
Resolver->>Parser: parse(package.json or tsconfig.json string)
Parser->>Parser: strip BOM if present
Parser->>Resolver: return deserialized config
Resolver->>Test: return resolved path
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
🧰 Additional context used🧬 Code Graph Analysis (1)src/tsconfig_serde.rs (2)
🪛 LanguageToolREADME.md[uncategorized] ~21-~21: Loose punctuation mark. (UNLIKELY_OPENING_PUNCTUATION) 🪛 GitHub Check: Codacy Static Code Analysisfixtures/misc/package-json-with-bom/package.json[warning] 1-1: fixtures/misc/package-json-with-bom/package.json#L1 ⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (10)
✨ 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:
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 (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces BOM (Byte Order Mark) support by stripping BOMs from JSON strings during configuration parsing, and it enhances package resolution by adjusting package specifier parsing. Key changes include:
- Adding tests for BOM handling in package.json and tsconfig paths.
- Implementing BOM stripping in tsconfig_serde.rs using a custom helper.
- Modifying the package specifier parsing in src/lib.rs to pass package name and subpath separately.
Reviewed Changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/resolve_test.rs | Added a test for BOM handling in package-json resolution. |
| src/tsconfig_serde.rs | Introduced a custom BOM stripping function and integrated its call in parsing. |
| src/tests/tsconfig_paths.rs | Added a test case for BOM presence in tsconfig paths. |
| src/package_json_serde.rs | Updated BOM removal to use the standard library function. |
| src/lib.rs | Adjusted package specifier parsing and updated the node modules resolution call. |
Files not reviewed (2)
- fixtures/misc/package-json-with-bom/package.json: Language not supported
- fixtures/tsconfig/cases/with-bom/tsconfig.json: Language not supported
Comments suppressed due to low confidence (2)
src/tsconfig_serde.rs:272
- [nitpick] Consider using the standard library's trim_start_matches (as done in package_json_serde.rs) for BOM removal to maintain consistency across modules.
let json = trim_start_matches_mut(json, '\u{feff}'); // strip bom
src/tsconfig_serde.rs:281
- [nitpick] The function name 'trim_start_matches_mut' may be misleading since it does not modify the original string in place but rather returns a trimmed slice; consider renaming it to better reflect its behavior, for example 'trim_bom'.
fn trim_start_matches_mut(s: &mut str, pat: char) -> &mut str {
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 8faa583 in 1 minute and 48 seconds. Click for details.
- Reviewed
144lines of code in8files - Skipped
1files when reviewing. - Skipped posting
7draft 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. tests/resolve_test.rs:240
- Draft comment:
Good test for BOM; it correctly checks that package.json with BOM is trimmed and resolved properly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. tests/resolve_test.rs:3
- Draft comment:
Ensure module alias consistency: unrs_resolver and oxc_resolver are both referenced; consider using consistent naming. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/tsconfig_serde.rs:272
- Draft comment:
The result ofjson_strip_comments::strip(json)is discarded. It likely returns a new, comment‑free string that should be used for parsing. Consider assigning its result (e.g.let json = json_strip_comments::strip(json);) so that comments are removed before deserialization. - Reason this comment was not posted:
Comment was on unchanged code.
4. tests/resolve_test.rs:240
- Draft comment:
There is a mix of resolver result mappings using bothunrs_resolver::Resolutionandoxc_resolver::Resolution. Confirm that this is intentional and that the proper resolution type is used consistently in tests. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. fixtures/misc/package-json-with-bom/package.json:1
- Draft comment:
The file begins with a Byte Order Mark (BOM) before the '{'. While BOMs are sometimes used, they can introduce subtle issues in JSON files. Please ensure that this is intentional; if not, consider removing the BOM to keep the file clean. - 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% This appears to be a test fixture specifically created to test handling of package.json files with BOMs. The file name and location strongly suggest the BOM is intentional. Making a comment to remove an intentional test case feature would be counterproductive. I could be wrong about the intention - maybe this is just a poorly named file that accidentally got a BOM. The combination of the explicit filename 'package-json-with-bom' and its location in a 'fixtures' directory (commonly used for test cases) makes it extremely unlikely the BOM is accidental. The comment should be deleted as it suggests removing a BOM that appears to be an intentional part of a test fixture.
6. src/package_json_serde.rs:3
- Draft comment:
Typo: The comment on line 3 says 'Code related to export field are copied...' which should probably be 'Code related to export fields are copied...' for correct pluralization. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/package_json_serde.rs:192
- Draft comment:
Typo: The documentation comment starting on line 192 says 'large fields that useless for pragmatic use are removed.' Consider revising to 'large fields that are useless for pragmatic use are removed.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_o4wjAon1alPMT2Rk
You can customize 
CodSpeed Performance ReportMerging #94 will not alter performanceComparing Summary
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 93.38% 93.40% +0.02%
==========================================
Files 13 13
Lines 2856 2865 +9
==========================================
+ Hits 2667 2676 +9
Misses 189 189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed b6d70f6 in 1 minute and 36 seconds. Click for details.
- Reviewed
151lines of code in7files - Skipped
1files when reviewing. - Skipped posting
3draft 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. src/tsconfig_serde.rs:272
- Draft comment:
The call to json_strip_comments::strip(json) discards its result. If the intent is to remove comments, consider reassigning the result (e.g. let json = json_strip_comments::strip(json);) so that the cleaned JSON is parsed. - Reason this comment was not posted:
Comment was on unchanged code.
2. src/tsconfig_serde.rs:272
- Draft comment:
The call to json_strip_comments::strip(json) discards its result—this means comments aren’t actually removed. Consider assigning its return value (e.g. let stripped = json_strip_comments::strip(json)) and then parsing from the stripped string. - Reason this comment was not posted:
Comment was on unchanged code.
3. README.md:13
- Draft comment:
Typographical Issue: 'Raspberry PI 4 aarch64' should be 'Raspberry Pi 4 aarch64' to reflect the correct product naming. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_TnPTbgZgJndiKTkB
You can customize
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Important
This PR adds support for handling BOM in
package.jsonandtsconfig.jsonby stripping it during parsing and updates tests to verify this behavior.package.jsonandtsconfig.jsonby stripping it during parsing insrc/package_json_serde.rsandsrc/tsconfig_serde.rs.load_node_modules()insrc/lib.rsto acceptpackage_nameandsubpathparameters.with_bom()test insrc/tests/tsconfig_paths.rsto verify BOM handling intsconfig.json.package_json_with_bom()test intests/resolve_test.rsto verify BOM handling inpackage.json.README.mdto include reference to the new upstream merge#94.This description was created by
for b6d70f6. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
package.jsonandtsconfig.json.