Revert "feat(bun): remove mjs path resolution" #347
Conversation
This reverts commit 073d86b.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
WalkthroughThe changes involve modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/unplugin-typia/src/bun.ts(3 hunks)
🔇 Additional comments (2)
packages/unplugin-typia/src/bun.ts (2)
152-161: Verify dependency on typia#1413.
Since this is a revert PR dependent on typia#1413, we should ensure that PR is merged and the revert is still necessary.
Let's check the status of the dependent PR:
17-24: LGTM! Well-documented type extension.
The type definition clearly explains the purpose of forceImportTypiaMjs and the CommonJS compatibility issue it addresses.
Let's verify if typia's package.json indeed has "type": "commonjs":
✅ Verification successful
The forceImportTypiaMjs option is correctly implemented and documented
The implementation aligns with the documented behaviour:
- The option defaults to
trueas stated in the type definition - It's used in the Bun plugin to transform typia's
.jsimports to.mjs - The code handles the module resolution issue by intercepting loads from
typia/liband replacing the extensions
Even though we couldn't directly verify typia's package.json, the implementation and usage context provide sufficient evidence that this is addressing a real compatibility issue between Bun and typia's module system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify typia's package.json type field
rg '"type":\s*"commonjs"' node_modules/typia/package.json
Length of output: 131
Script:
#!/bin/bash
# Let's check if typia is a direct dependency and find its package.json
fd package.json --type f --exec grep -l "typia" {}
# Also search for any direct references to typia's package.json type field in the codebase
rg -i "typia.*package\.json.*type.*commonjs"
Length of output: 508
Script:
#!/bin/bash
# Let's check the actual implementation to understand how this option is used
ast-grep --pattern 'forceImportTypiaMjs'
# Also check if there are any related test cases or examples
rg 'forceImportTypiaMjs' -A 3
Length of output: 996
|
|
||
| /** if input is ./node_modules/typia/lib/*, convert js to mjs */ | ||
| if (forceImportTypiaMjs) { | ||
| build.onLoad({ filter: /.+\/node_modules\/typia\/lib\/.*\.js$/ }, async (args) => { | ||
| const { path } = args; | ||
| const mjsPath = path.replace(/\.js$/, '.mjs'); | ||
|
|
||
| return { contents: await Bun.file(mjsPath).text() }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for missing .mjs files.
The current implementation assumes the .mjs file will always exist. Consider adding error handling to gracefully handle cases where the .mjs file is missing.
Here's a suggested improvement:
if (forceImportTypiaMjs) {
build.onLoad({ filter: /.+\/node_modules\/typia\/lib\/.*\.js$/ }, async (args) => {
const { path } = args;
const mjsPath = path.replace(/\.js$/, '.mjs');
-
- return { contents: await Bun.file(mjsPath).text() };
+ try {
+ const file = Bun.file(mjsPath);
+ if (await file.exists()) {
+ return { contents: await file.text() };
+ }
+ console.warn(`MJS file not found: ${mjsPath}, falling back to JS`);
+ return { contents: await Bun.file(path).text() };
+ } catch (error) {
+ console.error(`Error loading file: ${mjsPath}`, error);
+ throw error;
+ }
});
}📝 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.
| /** if input is ./node_modules/typia/lib/*, convert js to mjs */ | |
| if (forceImportTypiaMjs) { | |
| build.onLoad({ filter: /.+\/node_modules\/typia\/lib\/.*\.js$/ }, async (args) => { | |
| const { path } = args; | |
| const mjsPath = path.replace(/\.js$/, '.mjs'); | |
| return { contents: await Bun.file(mjsPath).text() }; | |
| }); | |
| } | |
| /** if input is ./node_modules/typia/lib/*, convert js to mjs */ | |
| if (forceImportTypiaMjs) { | |
| build.onLoad({ filter: /.+\/node_modules\/typia\/lib\/.*\.js$/ }, async (args) => { | |
| const { path } = args; | |
| const mjsPath = path.replace(/\.js$/, '.mjs'); | |
| try { | |
| const file = Bun.file(mjsPath); | |
| if (await file.exists()) { | |
| return { contents: await file.text() }; | |
| } | |
| console.warn(`MJS file not found: ${mjsPath}, falling back to JS`); | |
| return { contents: await Bun.file(path).text() }; | |
| } catch (error) { | |
| console.error(`Error loading file: ${mjsPath}`, error); | |
| throw error; | |
| } | |
| }); | |
| } |
This reverts commit 073d86b.
after samchon/typia#1413 is merged, merge this PR
Summary by CodeRabbit
.jsto.mjs.