Use additional npm / Yarn options to speed up installation #128
Conversation
| npm = ["yarn"] if is_yarn else ["npm"] | ||
|
|
||
| npm_cmd = normalize_cmd(npm) | ||
|
|
||
| install_cmd = ["install", "--immutable"] if is_yarn else ["install", "--no-audit", "--no-fund"] |
There was a problem hiding this comment.
It might be reasonable to use npm ci instead of npm install. npm ci is intended for automated environments where determinism is crucial, and it's roughly equivalent to yarn install --immutable.
However, npm ci does remove and reinstall node_modules. With package download caches, this doesn't use additional bandwidth, but it's definitely slower than npm install.
Do we care more about absolute correctness or speed when building internal JS assets for a Python package?
There was a problem hiding this comment.
I'm wary of changing any of this default behavior, as I'm not a JS developer anymore and I'm not sure what affect this would have on existing consumers.
There was a problem hiding this comment.
Ok, are you comfortable with this change as-is?
--no-fundonly changes what's written to stdout, to make it cleaner in logs--no-auditspeeds up the command by preventing a background HTTP request and prevents additional output to stdout- For Yarn,
--immetableis already the behavior when this is run in CI (determined via environment variables), so explicitly setting it makes the hook more consistent across all environments. Substantively, it's an important check, as it ensures that theyarn.lockwill not be modified by the hook, which is very important to deterministic package builds.
| @@ -94,25 +94,27 @@ def npm_builder( | |||
| if isinstance(npm, str): | |||
| npm = [npm] | |||
|
|
|||
| is_yarn = (abs_path / "yarn.lock").exists() | |||
There was a problem hiding this comment.
It's always been possible that this heuristic is incorrect, but the risk is slightly magnified now that we rely on it even if npm (the executable location) is explicitly provided.
I don't see a better alternative though, without attempting to look within npm to do pattern-matching for "yarn", which could still be incorrect (e.g. npm = "/home/john.yarn/bin/npm").
There was a problem hiding this comment.
Specifically this change is what I am uncertain about.
The node_modules installation should be as fast and deterministic as possible, as it's taking place internally within a Python package build. Routine developer-facing information should be omitted, as this is expected to be handled as part of the direct Javascript development process. See: https://docs.npmjs.com/cli/v7/commands/npm-install#configuration https://yarnpkg.com/cli/install#options
brianhelba
force-pushed
the
npm-install
branch
from
a170a62 to
8897dba
Compare
|
@blink1073 Is this something that you'll be interested in reviewing? |
The
node_modulesinstallation should be as fast and deterministic as possible, as it's taking place internally within a Python package build. Routine developer-facing information should be omitted, as this is expected to be handled as part of the direct Javascript development process.See:
https://docs.npmjs.com/cli/v7/commands/npm-install#configuration https://yarnpkg.com/cli/install#options