Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

Add {complete:true} buildIdealTree option to load sw/bundles #73

Closed
wants to merge 1 commit into from

Conversation

isaacs
Copy link
Contributor

@isaacs isaacs commented May 9, 2020

Needed for implementing 'npm install --package-lock-only'

Needed for implementing 'npm install --package-lock-only'
Copy link
Contributor

@ruyadorno ruyadorno left a comment

Choose a reason for hiding this comment

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

one more question:

  • Where did arborist landed with regards to bundledDependencies name variation? is it not supported? or is it just converted in some abstraction i might have missed?

@@ -34,7 +35,9 @@ class Arborist extends Auditor(require('events')) {
nodeVersion: process.version,
...options,
path: options.path || '.',
cache: options.cache || `${homedir()}/.npm/_cacache`,
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like it would be nicer to have a cacache API that returns the default location instead 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.

Hmm... really depends if we want that to be a "cacache default" vs an "npm default". It's kind of nice for cacache to just be agnostic about any npm-specific stuff, and ~/.npm/_cacache definitely feels npm-specific.

@@ -8,6 +8,7 @@
"@npmcli/name-from-folder": "^1.0.1",
"@npmcli/run-script": "^1.3.1",
"bin-links": "^2.1.2",
"cacache": "^15.0.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why cacache is only getting added now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Until now, Arborist didn't use cacache directly! But cracking open tarballs requires a temp directory to extract into, and it just makes sense to use the temp directory that cacache provides, since the rest of npm does.


if (hasShrinkwrap)
await new Arborist({ ...this.options, path })
.loadVirtual({ root: node })
Copy link
Contributor

Choose a reason for hiding this comment

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

wow are these composing install trees? 🤩 is it that easy? it gives me some workspaces ideas...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! #72 is an open issue to add a similar facility to loadActual(), which will make the loading of bundle deps a bit easier.

@isaacs
Copy link
Contributor Author

isaacs commented May 13, 2020

Where did arborist landed with regards to bundledDependencies name variation? is it not supported? or is it just converted in some abstraction i might have missed?

The canonical name is bundleDependencies, no dD. This is enforced by read-package-json-fast here, and by read-package-json here. It occurs to me that we don't check for that in the manifests we get from the registry via pacote, which I'll go add now. (Since it's been enforced at publish time for many years now, it's probably fine, but still, good to be extra careful.)

@ruyadorno ruyadorno closed this in b28a43f May 14, 2020
@wraithgar wraithgar deleted the isaacs/build-ideal-complete branch April 22, 2021 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants