Standardize and normalize packages #21
Conversation
zkat
force-pushed
the
zkat/normalized-packages
branch
3 times, most recently
from
23ef7cf
to
b2dea92
Compare
zkat
force-pushed
the
zkat/normalized-packages
branch
from
c06a302
to
04a0659
Compare
zkat
force-pushed
the
zkat/normalized-packages
branch
from
04a0659
to
fe82603
Compare
lib/finalize-manifest.js
Outdated
| // This one depends entirely on each handler. | ||
| this._resolved = pkg._resolved | ||
|
|
||
| // We sometimes need to grab these from the tarball. |
There was a problem hiding this comment.
I'll expand on this. I think it's worth explaining.
lib/finalize-manifest.js
Outdated
| this.bin = pkg.bin || fromTarball.bin || null | ||
|
|
||
| if (this.bin && Array.isArray(this.bin)) { | ||
| // This isn't even documented? |
There was a problem hiding this comment.
Is this comment necessary?
| // I don't want this why did you give it to me. Go away. 🔥🔥🔥🔥 | ||
| delete this.readme | ||
|
|
||
| // Object.freeze(this) |
There was a problem hiding this comment.
Do you need this comment? :)
There was a problem hiding this comment.
This is a reminder to myself of something I really want to do but npm doesn't let me do yet -- it'll be incommented once I figure a few things out, and I didn't want it to be forgotten.
| log: npmlog | ||
| } | ||
|
|
||
| test('basic extraction') |
There was a problem hiding this comment.
If it hasn't already, let's create an issue to finish these tests lest we forget.
There was a problem hiding this comment.
Not gonna forget. These are very visible if you run npm t. Also, #48 is the PR adding these tests (and registry.tarball.js)
zkat
force-pushed
the
zkat/normalized-packages
branch
from
fe82603
to
113d7fb
Compare
|
The bin thing totally is documented. See the
|
I would rather keep util for extractable/reusable/generic utilities. extract-stream has domain-specific knowledge.
fix(manifest): prefer shasum in dist fix(finalize-anifest): npm really does expect bundleDeps to be an array refactor(finalize-manifest): completeFromTarball -> tarballedProps and stop making it side-effectful. fix(manifest): no need for where fix(finalize-manifest): default this out to simplify tests refactor(finalize-manifest): use external checksum-stream fix(finalize-manifest): stop freezing, delete readme, put bin in extraProps feat(finalize-manifest): handle array bin field
zkat
force-pushed
the
zkat/normalized-packages
branch
from
113d7fb
to
d7ed77e
Compare
| manifest._shasum = ( | ||
| manifest.dist && manifest.dist.shasum | ||
| ) || manifest._shasum | ||
| manifest._resolved = ( |
There was a problem hiding this comment.
Your lisp accent is showing. 😉 I feel like these expressions are waaay too big to be easy to follow in JS. If you don't want to do if-branched assignment then maybe put the different alternatives in functions?
Fixes #17 and #18.
With this PR,
pacote.manifestwill now return a single, immutableManifestobject that has a consistent set of fields, with normalized data and semantics. These fields are chosen based on their utility to the installer. Things that are missing are thus not considered necessary for installation.Aside from a few handler-specific details, there's now a single place in the code where we do fallback to the package tarballs in order to fill in missing/unknown fields, including
_shrinkwrap,bin, and even_shasum.TODO
finalize-manifest. Include tests to check the bypassing of tarball extractsextract-shrinkwraptests to be used byfinalize-manifesttests.