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

fix: removes [_complete] check to apply correct metadata #287

Closed
wants to merge 3 commits into from
Closed

fix: removes [_complete] check to apply correct metadata #287

wants to merge 3 commits into from

Conversation

gijun19
Copy link
Contributor

@gijun19 gijun19 commented Jun 4, 2021 •
edited by isaacs
Loading

removes a check for [_complete] which determines whether or not we reify instead unpacking the tarball and looking inside. @isaacs raised some concerns about some slower install times potentially and how this check could be unnecessary. probably want to discuss this as a team.

CC: @darcyclarke

References

Closes npm/cli#2606
@gijun19 gijun19 mentioned this pull request Jun 4, 2021
Open
@isaacs
Copy link
Contributor

isaacs commented Jun 4, 2021

Need to re-generate snapshots and add a semver tarball to the mock registry.
@bl-ue
Copy link

bl-ue commented Jun 4, 2021

Closes #2606

@gimli01 you mean

Closes npm/cli#2606
@isaacs
Copy link
Contributor

isaacs commented Jun 4, 2021

Since we're now always inflating the old (not-ancient) lockfile in buildIdealTree, there's no need to fetch missing package details in reify(). Fixup commit:

  • adds missing semver tarball fixture
  • updates snapshots
  • removes now-dead code in lib/arborist/reify.js
@isaacs isaacs requested a review from nlf June 4, 2021 16:23
@gijun19
Copy link
Contributor Author

gijun19 commented Jun 4, 2021

@isaacs looks like one pipeline failed to run
@darcyclarke darcyclarke mentioned this pull request Jun 4, 2021
Closed
nlf
nlf approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@nlf nlf left a comment

Choose a reason for hiding this comment

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

this makes sense, the changes all look good. i'm a little curious what the performance impact will be, but since it's likely a one time cost when updating an old lockfile i think that's a really minor concern
@nlf
Copy link
Contributor

nlf commented Jun 4, 2021

@isaacs looks like one pipeline failed to run

one of the tests caused node to crash with exit code 3221225477 which translates to 0xC0000005 in hex and represents an access violation. it's pretty likely this failure is due to an issue with the host that ran the test, and not with our code. i went ahead and reran everything though to see if it clears up
@isaacs isaacs closed this in ff35994 Jun 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
4 participants
@gijun19 @isaacs @bl-ue @nlf