Skip to content
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

fix: add .cmd to commands on windows #124

Merged
merged 2 commits into from
Jan 29, 2021
Merged

fix: add .cmd to commands on windows #124

merged 2 commits into from
Jan 29, 2021

Conversation

nathanhleung
Copy link
Owner

This PR hopefully resolves #123. Here is what I think is going on:

  1. Before PR use package manager to get info #85, which constitutes the main set of changes in v3.0.0, spawnCommand (a thin install-peerdeps-specific wrapper around child_process.spawn to promisify it) was called only in the install step (the call was something like spawnCommand("npm", ["install", "eslint-config-airbnb", ...restOfPeerdeps]).
  2. The changes in use package manager to get info #85 introduced a second call to spawnCommand, to get package info directly from the package manager (through some code along the lines of spawnCommand("npm", ["info", "eslint-config-airbnb"]).
  3. There was some special handling in the install step code, outside of spawnCommand, to preprocess the first argument to spawnCommand on Windows due to child_process.spawn does not work with npm run scripts on windows. nodejs/node#3675. Namely, the call on Windows would be changed to something like spawnCommand("npm.cmd", ["install", "eslint-config-airbnb"]) (note the .cmd suffix added to the first argument).
  4. However, this special first argument handling was not copied to the package info retrieval code in use package manager to get info #85, which I think is what led to ERR spawn npm ENOENT #123 and breakage in v3.0.0 being reported on Windows.
  5. This PR moves the special first argument handling inside of spawnCommand, automatically adding the .cmd suffix if it's not already present if it detects the tool running on Windows, which hopefully fixes ERR spawn npm ENOENT #123 and prevents a bug like this from happening again in the future.

Copy link
Collaborator

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super gross hack, but this is a cleaner way to do an existing hack, so, nice :-)

@karlhorky
Copy link

Ah right, the .cmd extension on Windows - I read of that in my research before I found #123 ! Sounds promising - @thomalexg and I will take a look tomorrow.

@karlhorky
Copy link

karlhorky commented Jan 27, 2021

@nathanhleung what do you think about adding Windows end-to-end tests to install-peerdeps? To catch any errors like this in the future? GitHub Actions and seemingly also Travis CI have the ability to run on Windows...

@ljharb
Copy link
Collaborator

ljharb commented Jan 27, 2021

travis-ci is slow and dying; i'd suggest github actions. I can convert the existing tests to github actions if @nathanhleung is OK with that; windows tests can go in separately/in parallel as a separate workflow.

@karlhorky
Copy link

karlhorky commented Jan 27, 2021

Cool, if this is the way to go, maybe that could be done with a matrix - here's our approach for Preflight: https://github.com/upleveled/preflight/blob/main/.github/workflows/build-lint-test.yml#L7-L10

@nathanhleung
Copy link
Owner Author

@nathanhleung what do you think about adding Windows end-to-end tests to install-peerdeps? To catch any errors like this in the future? GitHub Actions and seemingly also Travis CI have the ability to run on Windows...

I think that's a great idea — don't want to break this package for Windows users in the future if I don't have to.

travis-ci is slow and dying; i'd suggest github actions. I can convert the existing tests to github actions if @nathanhleung is OK with that; windows tests can go in separately/in parallel as a separate workflow.

It would be awesome if you could do that, thanks!

@karlhorky
Copy link

Hi @karlhorky, opened #124 which I think should fix but can't test on macOS. Could you git fetch and then git checkout nathanhleung-fix and let me know whether it works?

The branch is no longer throwing the ERR spawn npm ENOENT / ERR spawn yarn ENOENT errors:

$ git fetch
$ git checkout nathanhleung-fix
$ yarn build && node . --yarn --dev @upleveled/eslint-config-upleveled && git reset --hard HEAD
yarn run v1.22.5
$ npm run clean
> install-peerdeps@3.0.0 clean
> rimraf lib
$ babel src --out-dir lib --ignore *.test.js
Successfully compiled 9 files with Babel (1411ms).
✨  Done in 3.14s.
install-peerdeps v3.0.0
Installing peerdeps for @upleveled/eslint-config-upleveled@latest.
yarn add @upleveled/eslint-config-upleveled@1.5.6 eslint-config-react-app@6.0.0 eslint-import-resolver-typescript@2.3.0 eslint-plugin-cypress@2.11.2 eslint-plugin-unicorn@26.0.1 typescript@4.1.3 --dev
SUCCESS @upleveled/eslint-config-upleveled
  and its peerDeps were installed successfully.
HEAD is now at 59c129e build: remove extraneous dependencies. resolves #115

Seems good to go! 🛳

@CoachRDeveloper
Copy link

Confirmed that it works on Windows. Please complete this pull request.

@nathanhleung
Copy link
Owner Author

Hi @karlhorky, opened #124 which I think should fix but can't test on macOS. Could you git fetch and then git checkout nathanhleung-fix and let me know whether it works?

The branch is no longer throwing the ERR spawn npm ENOENT / ERR spawn yarn ENOENT errors:

$ git fetch
$ git checkout nathanhleung-fix
$ yarn build && node . --yarn --dev @upleveled/eslint-config-upleveled && git reset --hard HEAD
yarn run v1.22.5
$ npm run clean
> install-peerdeps@3.0.0 clean
> rimraf lib
$ babel src --out-dir lib --ignore *.test.js
Successfully compiled 9 files with Babel (1411ms).
✨  Done in 3.14s.
install-peerdeps v3.0.0
Installing peerdeps for @upleveled/eslint-config-upleveled@latest.
yarn add @upleveled/eslint-config-upleveled@1.5.6 eslint-config-react-app@6.0.0 eslint-import-resolver-typescript@2.3.0 eslint-plugin-cypress@2.11.2 eslint-plugin-unicorn@26.0.1 typescript@4.1.3 --dev
SUCCESS @upleveled/eslint-config-upleveled
  and its peerDeps were installed successfully.
HEAD is now at 59c129e build: remove extraneous dependencies. resolves #115

Seems good to go! 🛳

Great! I'll cut 3.0.1 and push to NPM

@nathanhleung nathanhleung merged commit 9c3416b into master Jan 29, 2021
@nathanhleung nathanhleung deleted the nathanhleung-fix branch January 29, 2021 03:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ERR spawn npm ENOENT
4 participants