RFC: Only Registry Dependencies

[thescientist13]

When installing dependencies, the npm CLI should have a mechanism for communicating (and optionally failing on) dependencies that do not come from a registry.

References

related to #581

[ljharb]

Does this cover only direct dependencies? What about transitives?

[ljharb]

also, it's not about tarballs, it's about "from a registry" - users don't think about tarballs, and a tarball URL from a registry should be disallowed too.

[thescientist13]

Feedback from the meeting for updating this RFC

1. Tie this in with npm audit instead of install
2. Warn by default
3. Reference npm deprecate for output messaging example / conventions
4. Rename to --only-non-remote-deps
5. Ensure proper communication / documentation with the rollout

Perhaps in the future as another RFC, we can leverage npm query to add more granularity / overrides to this command by being able to "filter" (include / exclude) on dependencies, devDependencies, etc

cc: @ljharb / @darcyclarke to keep me honest ^ 🙏

[ljharb]

re 4, npm audit dependency-types, or something similar - and then have install auto-run that audit subcommand - and then command-specific config can be used to customize it using an npm query selector

[dominykas]

Not sure if this needs to be explicitly stated, but since I just recently had to do the work internally - the warning should also apply to overrides (quite possibly the same logic as transient vs direct deps - although an override could be either).

[thescientist13]

Ok, think I covered the feedback here in a follow up revision

1. Linked to npm audit and touched upon interplay between npm install
2. Default option set to warn
3. Leverage npm why style output within the Implementation section
4. Captured future examples of future eslint style audits / filters that could be applied (as new RFC) in the Bikeshedding section

For the next meeting, would just like to take another pass at the naming of the flag ( --only-non-remote-deps ) and overall RFC name to see if another coat of pain on the bike shed is desired. I know the emphasis here is on registries specifically and not so much tarballs (which I agree with) but wanted to capture a couple of my own personal thoughts on the flag name:

• As I read it, it seems to assert on the negative which is kind of hard to reason about, like an if(!someNegativeBool)? Feel like I have to keep reading it and reversing it to get the right "state" in my mind.
• It doesn't mention anything about registries, but this flag strongly correlates with that as the principal source of truth, and so wondering if discoverability (in docs, google, etc) would be hampered by too abstract of a name?
• To the point above, I believe one call out was that a locally linked specifier would be an exception. Is that the only case? Maybe that's just more of an implicit localhost style local development convenience that could be documented, rather than pull the name in a more abstract direction? (e.g. centering the name on principal use case on registry specifiers first and foremost).

Happy to make any updates to the RFC title, filename, etc if it's ok to have another chat on the name and thanks again for all the help and feedback on this so far! 🙌

[wesleytodd]

pass at the naming of the flag (--only-non-remote-deps)

Thoughts on --only-registry-deps? I think this more accurate to the intent right? Specifically this is intended to flag "mutable deps", and since we trust the registry to be immutable (mostly) it seems reasonable to me to name it this. And it seems folks above agree with this.

---

FWIW, this conversation is why I think we should consider following a well known path for this problem. We would need to have discussions like this for each of these new additions and they are gated by a central authority. The eslint model is federated by design and the most common & agreed upon rules bubble up to the core while letting innovation happen in plugins and opinionated rule-sets. The npm team and npm cli do not need to be the ultimate gate keeper, and shouldn't be for their own time management optimization.

[thescientist13]

Action Items / Takeaways

1. I should review npm query and understand how to express this flag through that, as the expectation is given the trajectory of query, that it will land prior to this and should be considered the principal mechanism for enabling this feature
2. From there, express that updated expectation in this RFC, to cite delegation to npm query

For testing query as it is not out yet, I can

1. Clone the CLI repo
2. Checkout PR 5000
3. Link the CLI (to itself)

$ gh repo clone npm/cli && cd npm/cli && gh pr checkout 5000 && npm link .
// something like that ^ :)

[thescientist13]

Had some time to play around with the above and got npm query working locally and created a test bench repo to practice with.

For example, was able to single out the eslint dependency as such from this package.json

{
  "name": "npm-query-tarball-test",
  "dependencies": {
    "@babel/cli": "^7.4.0",
    "eslint": "git+https://github.com/eslint/eslint.git"
  }
}

% npm query ":root > *[resolved^="git"]"
[
  {
    "version": "8.20.0",
    "resolved": "git+ssh://git@github.com/eslint/eslint.git#0bcd2255c40b5c115a95181864776b0dd456c2dc",
    "license": "MIT",
    "dependencies": {
      "@eslint/eslintrc": "^1.3.0",
      "@humanwhocodes/config-array": "^0.9.2",
      "ajv": "^6.10.0",
      "chalk": "^4.0.0",
      "cross-spawn": "^7.0.2",
      "debug": "^4.3.2",
      "doctrine": "^3.0.0",
      "escape-string-regexp": "^4.0.0",
      "eslint-scope": "^7.1.1",
      "eslint-utils": "^3.0.0",
      "eslint-visitor-keys": "^3.3.0",
      "espree": "^9.3.2",
      "esquery": "^1.4.0",
      "esutils": "^2.0.2",
      "fast-deep-equal": "^3.1.3",
      "file-entry-cache": "^6.0.1",
      "functional-red-black-tree": "^1.0.1",
      "glob-parent": "^6.0.1",
      "globals": "^13.15.0",
      "ignore": "^5.2.0",
      "import-fresh": "^3.0.0",
      "imurmurhash": "^0.1.4",
      "is-glob": "^4.0.0",
      "js-yaml": "^4.1.0",
      "json-stable-stringify-without-jsonify": "^1.0.1",
      "levn": "^0.4.1",
      "lodash.merge": "^4.6.2",
      "minimatch": "^3.1.2",
      "natural-compare": "^1.4.0",
      "optionator": "^0.9.1",
      "regexpp": "^3.2.0",
      "strip-ansi": "^6.0.1",
      "strip-json-comments": "^3.1.0",
      "text-table": "^0.2.0",
      "v8-compile-cache": "^2.0.3"
    },
    "bin": {
      "eslint": "bin/eslint.js"
    },
    "engines": {
      "node": "^12.22.0 || ^14.17.0 || >=16.0.0"
    },
    "funding": {
      "url": "https://opencollective.com/eslint"
    },
    "name": "eslint",
    "_id": "eslint@8.20.0",
    "pkgid": "eslint@8.20.0",
    "location": "node_modules/eslint",
    "path": "/Users/owenbuckley/Workspace/github/repos/npm-query-tarball-test/node_modules/eslint",
    "realpath": "/Users/owenbuckley/Workspace/github/repos/npm-query-tarball-test/node_modules/eslint",
    "from": [
      "",
      "node_modules/eslint-utils"
    ],
    "to": [
      "node_modules/@eslint/eslintrc",
      "node_modules/@humanwhocodes/config-array",
      "node_modules/ajv",
      "node_modules/eslint/node_modules/chalk",
      "node_modules/cross-spawn",
      "node_modules/debug",
      "node_modules/doctrine",
      "node_modules/eslint/node_modules/escape-string-regexp",
      "node_modules/eslint-scope",
      "node_modules/eslint-utils",
      "node_modules/eslint-visitor-keys",
      "node_modules/espree",
      "node_modules/esquery",
      "node_modules/esutils",
      "node_modules/fast-deep-equal",
      "node_modules/file-entry-cache",
      "node_modules/functional-red-black-tree",
      "node_modules/eslint/node_modules/glob-parent",
      "node_modules/eslint/node_modules/globals",
      "node_modules/ignore",
      "node_modules/import-fresh",
      "node_modules/imurmurhash",
      "node_modules/is-glob",
      "node_modules/js-yaml",
      "node_modules/json-stable-stringify-without-jsonify",
      "node_modules/levn",
      "node_modules/lodash.merge",
      "node_modules/minimatch",
      "node_modules/natural-compare",
      "node_modules/optionator",
      "node_modules/regexpp",
      "node_modules/strip-ansi",
      "node_modules/strip-json-comments",
      "node_modules/text-table",
      "node_modules/v8-compile-cache"
    ],
    "dev": false,
    "inBundle": false
  }
]

So will just keep tinkering around with the query command and crafting the API to update this RFC with. Will look forward to a few minutes to discuss progress and next steps on the next RFC call.

[thescientist13]

From the call, got tipped off to these type hints that I can use with query to get specific information about the type of package, e.g. remote, version, git, etc.

The takeaway was that the only types that should be initially blocked are git and remote.

example:

npm query ":root > *:not(:type(git,remote))"

So, should have enough info now to get this RFC updated accordingly. 👍

[thescientist13]

Ok, new round of feedback applied! 🙌

• Aligned this RFC with the landing of (e.g. having a dependency on) npm query
• Updated Implementation section to detail delegation of the implementation to npm query. Suggesting the following query

  $ npm query ":root > *:is(:type(git,remote))"

• RFC name change to Only Registry Dependencies (was Only Registry Tarballs)
• Flag name change from --only-non-remote-deps to --only-registry-deps

---

Question

1. Does there need to be a distinction, either in implementation or query syntax, to account for Workspaces? Not sure if :root selector covers this or not or if it handles Workspaces implicitly? (My guess would be that's its "scope" is whatever that is of npm audit, but I'm not sure what that scope is currently)

[wraithgar]

:is(:type(git,remote)) would give you everything in your tree that is a git or remote reference.

[thescientist13]

OK, query updated! Think this might be good to go! 🙇

[ruyadorno]

I believe this line can be removed prior to ratifying / accepting the RFC

Suggested change

relates to #581

[ruyadorno]

Given that npm query has already landed I believe this note can be removed from the RFC text

Suggested change

> _**Note**: this RFC has a hard dependency on [`npm query`](https://github.com/npm/cli/pull/5000) landing to support its implementation._

[ruyadorno]

This section is usually removed prior to ratifying / accepting, are all these questions answered?

[thescientist13]

Yeah, I believe everything here is covered, so will clean this up now.

[thescientist13]

Ok, all notes and bikeshedding cleaned up!

[ljharb]

Here's the overall plan of record as I understand it:

npm audit will run multiple "audit types". One of these, of course, is "vulnerabilities", which is the current thing it checks. This RFC is for "dependencies" (name to be bikeshedded). Another one is "licenses" (#182). Many more can be envisioned, including a repeatable type that lets users provide a custom npm query selector.

Once command-specific config lands, each audit type will be configurable:

1. does it run in npm install, or npm audit, or both, or neither?
2. When it runs (in each of install or audit), does it fail the overarching action? or just warn?

This will allow users to set whatever level of strictness they want, separately for both audit and install, while simultaneously allowing npm maximal freedom to change the defaults in semver-majors however they like - because users who dislike the defaults can just configure it in advance.

[wraithgar]

Now that npm query can be told to exit uncleanly if any results come back with --no-expect-results I think npm has all the functionality needed for users to do this on their own.

[ljharb]

An example in the docs of how to do just that, especially in npmrc, would be amazing - and it’d still be nice and ergonomic (and help message that npm is encouraging this restriction) to have a single cli flag that wraps all the pieces.