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

rfc: Don't add metadata to package.json #38

Closed
wants to merge 1 commit into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented Aug 1, 2019

No description provided.

@isaacs isaacs force-pushed the isaacs/no-package-json-_fields branch 3 times, most recently from a73f413 to c35c78b Compare August 1, 2019 06:15

## Motivation

These fields are only relevant to npm, and as of version 6, npm relies on
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect; the requiredBy field has been incredibly invaluable when debugging dep graph issues. Could we find a way to have a simple answer to "what package(s) required package x"?

Perhaps a v6 minor that adds a CLI command that can show this info; then you could change the behind the scenes source of that info without breaking anyone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack! I forgot _requiredBy, yeah, that's one of the most interesting ones. I'll get to it tomorrow.

Could we find a way to have a simple answer to "what package(s) required package x"?

How would that differ from npm ls x today?

And don't worry, there is no way we're gonna land this before v7, it's definitely a breaking change :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess you're suggesting something where you can specify a single instance of a package folder in the tree, and it'd show all the cases where a dependency deduped to it?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like what I'm asking for - essentially, I want the answer to the question "why is this dependency here".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this PR to include _requiredBy and note the intention to create npm ls <location>, where <location> can be either a logical path, like /tap/react or a physical path like ./node_modules/tap/node_modules/react.

@isaacs isaacs force-pushed the isaacs/no-package-json-_fields branch from c35c78b to 90613e9 Compare August 2, 2019 21:12
isaacs added a commit that referenced this pull request Aug 2, 2019
Edit: add note about _requiredBy and `npm ls <location>`, prompted by
@ljharb in #38 (comment)
@isaacs isaacs force-pushed the isaacs/no-package-json-_fields branch from 90613e9 to 5bac23c Compare August 4, 2019 04:40
isaacs added a commit that referenced this pull request Aug 4, 2019
Edit: add note about _requiredBy and `npm ls <location>`, prompted by
@ljharb in #38 (comment)

Edit: add _shrinkwrap field.
@isaacs isaacs force-pushed the isaacs/no-package-json-_fields branch from 5bac23c to d9615e5 Compare August 15, 2019 22:59
@isaacs isaacs closed this in 2177cc9 Aug 15, 2019
@isaacs isaacs deleted the isaacs/no-package-json-_fields branch August 15, 2019 23:01
isaacs added a commit that referenced this pull request Aug 19, 2019
PR-URL: #38
Credit: @isaacs
Close: #38
Reviewed-by: @isaacs
@vweevers
Copy link

vweevers commented Sep 6, 2019

@isaacs When is this expected to land in npm? It will break prebuild tools that use _ fields to determine whether a package is installed as a dependency (npm i foo), standalone (cd foo && npm i) or a git dependency. We'll need some time to find an alternative solution.

Ref prebuild/prebuild-install#108

isaacs added a commit that referenced this pull request Sep 7, 2019
#38 (comment)

Finish the sentence that was incompl
@isaacs
Copy link
Contributor Author

isaacs commented Sep 7, 2019

@vweevers npm version 7 will have this change. It's breaking, for the reason you mention, basically -- anything depending on those fields, including npm@6, will no longer function properly with a tree installed by npm 7.

Time-wise, probably some time around end of year? Still a bit hard to say.

@vweevers
Copy link

vweevers commented Sep 8, 2019

Could npm, via npm-lifecycle, perhaps set environment variable(s) like npm_package_from? So that lifecycle scripts don't have to find & read node_modules/.arborist-resolutions.json.

The RFC states "These fields are only relevant to npm". However, tools like prebuild-install need access to the same information because they extend npm's functionality (until the day that npm itself can fill the gaps that exist in installing native modules).

@isaacs
Copy link
Contributor Author

isaacs commented Sep 9, 2019

@vweevers Exposing .arborist-metadata.json data to lifecycle scripts is a good idea.

For what it's worth, relying on some of those fields is really hazardous. They can be misleading, incorrect, or missing, even in normal operations. (And they're always missing for users who use yarn or pnpm.)

@vweevers
Copy link

Exposing .arborist-metadata.json data to lifecycle scripts is a good idea.

@isaacs Great! How do we achieve that? Us prebuild folks are willing to help (if you could give us some pointers on where to start, i.e. how do we get the metadata to npm-lifecycle, that would be much appreciated) unless the npm team can handle it.

For what it's worth, relying on some of those fields is really hazardous. They can be misleading, incorrect, or missing, even in normal operations. (And they're always missing for users who use yarn or pnpm.)

Roger. prebuild-install uses these fields to make an educated guess on whether to download or compile a native module, and has additional logic for yarn and pnpm. Worst case, when metadata is missing, prebuild-install chooses to download. In exotic cases where that decision is incorrect, the user can override it with --build-from-source. In other words: 60% of the time, it works every time.

@isaacs
Copy link
Contributor Author

isaacs commented Sep 10, 2019

@vweevers

In other words: 60% of the time, it works every time.

Lol, ok, great. Well, I've warned you about the hazards so my conscience is clear, but it sounds like you're aware.

How do we achieve that?

Let's start in this repo with a short RFC that references this one. tl;dr would be: when running any lifecycle script for package x/y/z, insert the data for that package from .arborist-metadata.json into the environment.

Is resolved and integrity enough? What other fields do you depend on, and which questions are you trying to answer with that data? That would be good to include in the RFC, just so we can ensure that we can be clear on the actual contract as development on Arborist moves forward. It may be that we can simplify or clarify your logic to bring that 60% up a bit higher.

@vweevers
Copy link

Is resolved and integrity enough? What other fields do you depend on, and which questions are you trying to answer with that data?

We currently use _from (and only _from, no other fields) to answer two questions, given the task of installing a package called my-native-module:

  1. Is my-native-module installed as a dependency (npm i my-native-module) or standalone (cd my-native-module && npm i)? In the former case, we download a prebuilt binary. In the latter case, we assume we're in a development workflow (of the author of my-native-module) and should therefore compile from source.
  2. Is my-native-module a git dependency (npm i owner/my-native-module)? If so, we assume the installed version has no prebuilt binaries and we should therefore compile from source.

In order to answer question 1, we currently detect the absence of _from but that's hazardous. If another field could answer it, we indeed have a opportunity here to improve the logic.

For question 2, exposing resolved would suffice.


/cc @lovell Could you confirm we're not using any other fields? I did a search across prebuild modules and only found _from.

@isaacs
Copy link
Contributor Author

isaacs commented Sep 10, 2019

I think the absence of resolved would answer question 1 more reliably, in fact. There won't be a resolved field if it's the root install project. For question 2 you could check to see if resolved is a git url or a registry tarball.

@vweevers
Copy link

SGTM!

@lovell
Copy link

lovell commented Sep 11, 2019

_from is the only soon-to-be-removed field I could find in the prebuild ecosystem too, so the suggestion of using resolved sounds like it will solve this, especially if made available via an environment/lifecycle variable.

node-pre-gyp is quite likely to run into this as well so we should probably externalise this logic (plus fallback) into a separate module for both to consume. (The combined weekly npm download count of prebuild-install and node-pre-gyp is about 7 million!)

@vweevers
Copy link

@lovell I scanned node-pre-gyp but couldn't find any usage of these fields.

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.

5 participants