-
-
Notifications
You must be signed in to change notification settings - Fork 62
feat(tarball): add shasum from tarball when appropriate #149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may want to add a similar exception for when the shasum is completely missing so we calculate it. See: https://github.com/zkat/pacote/blob/latest/lib/finalize-manifest.js#L143
@zkat - right! So I made the adjustment (and also adjusted some fixtures in the process). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests seem to be passing pretty consistently now.
Why are node streams so bad?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly looks good! One tiny change left that I missed 👍
lib/finalize-manifest.js
Outdated
@@ -188,7 +194,8 @@ function tarballedProps (pkg, spec, opts) { | |||
_resolved: (mani && mani._resolved) || | |||
(pkg && pkg._resolved) || | |||
spec.fetchSpec, | |||
_integrity: hash && hash.toString() | |||
_integrity: needsIntegrity && hash && hash.toString(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be hash.sha512.toString()
, since we don't currently put sha1s into the integrity
field.
Actually I'll just patch this myself real quick. This is good to merge in general. No need for more back-and-forth :) |
@zkat - that's great! Just one more suggestion if I may: With your change, maybe we can toss this line? https://github.com/zkat/pacote/pull/149/files#diff-d97a032389356a12ced608284b9c55edR169 |
oh! That's how you were doing it. I wondered why the tests were still passing. haha |
sorry for the rush -- didn't realize you were available and I just wanted to merge this, since I'm starting to put the release together. npm@6 is going out tomorrow :) |
I actually just happened to walk in through the door. :) |
omg this is what I get for editing everything on github |
Wait, doesn't everyone code like that? |
Heya!
This is the result some work done in relation to the discussion here: yarnpkg/yarn#5654
When I started writing it, I thought this was an issue but then saw the tests specifically looking for that
null
value (mentioning:// shasums are only when provided
). So I hope this is a wanted change! In my eyes it would be consistent with the rest of the behaviour. (I personally need this to facilitate lockfile conversion). But of course, if there's something I'm missing...Current Behavior:
After this PR: