Fix(npm): use require.resolve for binary path#1392
Fix(npm): use require.resolve for binary path#1392mixator wants to merge 1 commit intoorhun:mainfrom
Conversation
|
Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1392 +/- ##
==========================================
- Coverage 47.57% 47.53% -0.04%
==========================================
Files 24 24
Lines 2119 2119
==========================================
- Hits 1008 1007 -1
- Misses 1111 1112 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ognis1205
left a comment
There was a problem hiding this comment.
Please run the following commands to ensure the changes is valid:
yarn lintyarn buildyarn dev
I think there is another place we need to change. getExePath now returns the absolute path to the executable file instead of URL so:
return execa(fileURLToPath(exePath), args, {
stdio: "inherit",
...execaOptions,
});should be:
return execa(exePath, args, {
stdio: "inherit",
...execaOptions
});| import { createRequire } from "node:module"; | ||
| const require = createRequire(import.meta.url); |
There was a problem hiding this comment.
| import { createRequire } from "node:module"; | |
| const require = createRequire(import.meta.url); | |
| import { createRequire } from "node:module"; | |
| const require = createRequire(import.meta.url); |
For readability, I would prefer to insert newline here. It would be helpful to leave some comment as well, something like:
// Prepares `require` for ESM/CJS dual build.
const require = createRequire(import.meta.url);
| @@ -22,7 +24,7 @@ export async function getExePath() { | |||
|
|
|||
| try { | |||
| // Since the bin will be located inside `node_modules`, we can simply call import.meta.resolve | |||
There was a problem hiding this comment.
We should update this comment as well.
Description
Replaced NPM package binary import to support CJS and ESM
Motivation and Context
How Has This Been Tested?
Screenshots / Logs (if applicable)
Types of Changes
Checklist:
cargo +nightly fmt --allcargo clippy --tests --verbose -- -D warningscargo test