-
Notifications
You must be signed in to change notification settings - Fork 12
fix: shim non-existent source #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
10b779b
to
8855d40
Compare
I guess we can use this approach. |
8855d40
to
eff515f
Compare
I've rebased the PR on the recent changes to main. Not sure what's going on with the broken tests. Seems like the checks run on Windows, so I tried running it locally. I've run it on Windows 11 with both Node 14 and 18 and all tests pass for me. Any chance you might be willing to help push this across the finish line? Probably a lot easier for you given that you're more familiar with the code 😅 Currently I'm working around this by adding the following to my monorepo's root
Which runs this script:
I'd love to remove this hacky workaround from my code! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request addresses a PNPM issue where installations fail if a package’s bin file does not exist and cannot provide a shebang, by falling back on the extensionToProgramMap.
- Added a new test case for the scenario when the source file is missing.
- Modified the searchScriptRuntime function to handle ENOENT errors on Windows by checking for an alternative executable extension.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
test/test.js | Added a "no src file" test to cover the fallback behavior when the source file is missing. |
src/index.ts | Refactored the shebang extraction and error handling logic to support the new fallback scenario. |
Files not reviewed (1)
- test/snapshots/test.js.snap: Language not supported
Comments suppressed due to low confidence (2)
test/test.js:22
- Consider adding explicit assertions within the 'no src file' test case to verify that cmdShim correctly falls back to using the extensionToProgramMap when the source file is missing.
describe('no src file', () => {
src/index.ts:274
- Ensure that the fallback behavior when encountering an ENOENT error on Windows is fully intended – specifically, verify that relying on fs_.stat to check for an executable version aligns with the overall design, and consider adding a clarifying comment if needed.
if (isWindows() && err.code === 'ENOENT') {
if (isWindows() && err.code === 'ENOENT') { | ||
if (await opts.fs_.stat(`${target}${getExeExtension()}`)) { | ||
return { | ||
program: null, | ||
additionalArgs: '', | ||
} | ||
} | ||
} | ||
if (err.code !== 'ENOENT') throw err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The operation to compare err.code
to ENOENT
could be executed twice on Windows. You can optimize it by moving if (err.code !== 'ENOENT') throw err
up to right under catch
.
PNPM has this long-standing issue where pnpm install fails when a package defines a bin that must be compiled first: pnpm/pnpm#1801
In this case the file can't be read and so a shebang can't be determined, and execution fails with error code
ENOENT
. This seems easily solvable. cmd-shim can still fall back to determining the program by using theextensionToProgramMap
.This PR is probably incomplete. Not being terribly familiar with the code, I've done my best, but the test is probably incomplete at the very least.