formula: add std_npm_args

[branchvincent]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

While working on #17773, another node inconsistency that has bothered me is our npm args helper isn't exposed on the Formula class like all our other std_*_args. This fixes that by introducing a Formula.std_npm_args function, which for now just wraps the existing functions

Before:

require "language/node"
system "npm", "install", *Language::Node.local_npm_install_args
system "npm", "install", *Language::Node.std_npm_install_args(libexec)

After:

system "npm", "install", *std_npm_args(prefix: false)
system "npm", "install", *std_npm_args

Feedback welcome, if we like this I can also work on a rubocop to migrate to this new API

[chenrui333]

lgtm 👍

[branchvincent]

cc @homebrew/core for thoughts

[woodruffw]

LGTM, more consistency is always good.

(It'd be nice to get coverage on these new lines if possible, but not a blocker IMO if std_*_args is already hard to test.)

[alebcay]

Slight nit: when initially reading std_npm_args(prefix: false), it's unclear to me that prefix is a boolean (I assume controlling whether to install into prefix vs libexec) - from familiarity with other portions of the codebase, I would have expected prefix to be a String or Pathname. So I'd suggest considering a different name for the parameter? It could just be me though, if it makes sense to everyone else.

[branchvincent]

prefix does accept a string or pathname, false is just to disable the default of --prefix=libexec (which is the default since it's what we want 90% of the time, local_npm_install_args is only used in core 17 times):

> std_npm_args(prefix: "./foo")
=> ["-ddd",
 "--global",
 "--build-from-source",
 "--cache=/Users/branch/.cache/brew/npm_cache",
 "--prefix=./foo",
 "/tmp/test-1.0.0.tgz"]

> std_npm_args(prefix: false)
=> ["-ddd", "--build-from-source", "--cache=/Users/branch/.cache/brew/npm_cache"]

I copied what we did for std_pip_args

[Bo98]

Thanks!

[MikeMcQuaid]

Looks great, thanks @branchvincent!

[AThePeanut4]

@branchvincent This change causes "RuntimeError: npm failed to pack" errors for me, when trying to install a custom formula for basedpyright (a fork of pyright) that only uses Language::Node.local_npm_install_args. These specific lines cause the error (commenting them out fixes it):

brew/Library/Homebrew/formula.rb

Lines 2978 to 2979 in bdee21b

	when "npm"
	pretty_args -= std_npm_args

The problem occurs because std_npm_args forwards to Language::Node.std_npm_install_args, which then calls pack_for_installation, which then runs npm pack --ignore-scripts. In my case, npm pack fails with "npm error Invalid package, must have name and version", causing the "failed to pack" error message. But any npm command run from a directory that does not constitute a valid package will cause this error, in e.g. formula tests.

Also, unlike all the other std_*_args methods, which just directly return an argument list, std_npm_args has side effects - it sets an environment variable, modifies ./package.json, runs npm pack which generates a tarball, and mkdir's libexec/lib. Even if the npm pack command doesn't error, the side effects could potentially cause other issues, and since the call is directly in the system method, it can't be avoided. That is to say, I don't think system should be calling std_npm_args, unless std_npm_args is changed to just directly return an argument list.

If you'd like I can make a bug report, but I haven't yet been able to find a good way to trigger the issue with a core formula. Running brew install gulp-cli && brew test gulp-cli gives the same "failed to pack" error message, because it runs npm in the test method, but when I comment out the two lines I mentioned then I get a "unknown command: npm. Perhaps you have to reshim?" error.

[p-linnane]

@AThePeanut4 Please open an issue for further discussion.

[branchvincent]

Thanks for catching @AThePeanut4, fix in #17865 (ideally std_npm_args would be side-effect free but I don't want to introduce more changes than necessary)