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

[nodejs,angular] stop adding local node_modules/.bin to path #366

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

ryantm
Copy link
Collaborator

@ryantm ryantm commented Jun 27, 2024

Why

  • We were having issues where our local node_modules bin had a "which" binary that did not behave well.
  • It isn't typical for local node_modules/.bin to be on the path, if you want something on the path, you typically use npm install --global, which is still added to the PATH after this change.

What changed

  • Remove $REPL_HOME/node_modules/.bin from PATH

Test plan

  • Check template tester still passes
  • Check PATH does not contain that path in nodejs Repls

Rollout

Describe any procedures or requirements needed to roll this out safely (or check the box below)

  • This is fully backward and forward compatible

Why
===
* We were having issues where our local node_modules bin had a "which"
binary that did not behave well.
* It isn't typical for local node_modules/.bin to be on the path, if
you want something on the path, you typically use npm install
--global, which is still added to the PATH after this change.

What changed
===
* Remove $REPL_HOME/node_modules/.bin from PATH

Test plan
===
* Check template tester still passes
* Check PATH does not contain that path in nodejs Repls
@ryantm ryantm requested a review from a team as a code owner June 27, 2024 21:05
@ryantm ryantm requested review from airportyh and removed request for a team June 27, 2024 21:05
Copy link
Collaborator

@airportyh airportyh left a comment

Choose a reason for hiding this comment

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

Okay, that's true. People are pretty used to npx and the like.

@lafkpages
Copy link

I don't get how "This is fully backward and forward compatible" though. Existing Repls might have scripts that rely on local NPM binaries being in the PATH, and this would break them. And even if it only applies to new Repls, the same could happen for GitHub imports, right? You could try to import someone's old repo that had such a script and it would break.

I might be wrong though, I'm still trying to wrap my head around Nix.

@7heMech
Copy link
Contributor

7heMech commented Jun 28, 2024

It indeed is a breaking change, and I'm afraid some people might not know how to migrate, but it does indeed transfer to the expected behavior.

@ryantm
Copy link
Collaborator Author

ryantm commented Jun 28, 2024

Yes, you're right, this is a breaking change. I updated the checkmark.

https://docs.npmjs.com/cli/v10/configuring-npm/folders says

Any bin files are symlinked to ./node_modules/.bin/, so that they may be found by npm scripts when necessary.

we're pretty sure it is not conventional to put this dir on the user's PATH and we have evidence of this breaking at least one user, so we're going to move forward with it.

People can mitigate this themselves by adding an env section to their .replit if they need this:

[env]
PATH="$PATH:$REPL_HOME/node_modules/.bin"

@ryantm ryantm merged commit 9db943c into main Jun 28, 2024
1 check passed
@ryantm ryantm deleted the rtm-06-27-remove-local-node-modules-bin branch June 28, 2024 15:48
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.

None yet

4 participants