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.

Sign up for GitHub

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

Jump to bottom

[backport-v8.x] backport npm/npm@4ca6958 #18616

Closed
wants to merge 2 commits into from
Closed

[backport-v8.x] backport npm/npm@4ca6958 #18616

wants to merge 2 commits into from

Conversation

kasicka
Copy link

@kasicka kasicka commented Feb 7, 2018

Backporting npm/npm@4ca6958 ( 9f33a24 and b8888f5) that wasn't backported in #16509

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

MylesBorins and others added 2 commits February 7, 2018 10:10
@MylesBorins
deps: backport 4ca695819 from npm upstream
83056a3 
Original commit message:

    minizlib@1.0.4

    Fixes Node 9 compatibility.

    Credit: @isaacs

PR-URL: #16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins
deps: manually add 9.x support to npm
3387092 
refs: npm/npm#18964

PR-URL: #16509
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@nodejs-github-bot nodejs-github-bot added npm Issues and PRs related to the npm client dependency or the npm registry. v8.x labels Feb 7, 2018
richardlau
richardlau approved these changes Feb 7, 2018
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

cc @nodejs/lts

IMO we should include this in #18336. Because we floated these patches on master/v9.x they were missing from the subsequent upgrade npm to 5.6.0 PR #17535 / #17777 so when they were backported without these patches we ended up with files that don't match the equivalent in https://github.com/npm/npm/blob/v5.6.0.

@gibfahn
Copy link
Member

gibfahn commented Feb 7, 2018 •
edited
Loading

Why do we need to include the v9.x compatibility patch?

EDIT: Just read #16509 (comment), fair enough I guess.

@MylesBorins
Copy link
Contributor

So I'm leaning towards us simply relanding 5.6.0 following the instructions at https://github.com/nodejs/node/blob/master/doc/guides/maintaining-npm.md

Thoughts

@MylesBorins MylesBorins mentioned this pull request Feb 7, 2018
Closed
@MylesBorins
Copy link
Contributor

I've opened #18625 as an alternative. In it I completely relanded 5.6.0 following our maintaining npm guide

MylesBorins
MylesBorins suggested changes Feb 7, 2018
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment •
edited by gibfahn
Loading

Choose a reason for hiding this comment

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

I'd like to land #18625 instead as it seems less error prone

@kasicka thank you for opening this PR

@gibfahn
Copy link
Member

gibfahn commented Feb 8, 2018

So I'm leaning towards us simply relanding 5.6.0 following the instructions at /doc/guides/maintaining-npm.md@master

Thoughts

Sounds like a good idea to me.

@gibfahn
Copy link
Member

gibfahn commented Feb 8, 2018

Landed #18625, @kasicka thanks a lot for raising the PR and for telling us about the bug. Please take a look at the v8.x-staging branch (or the next RC) and make sure it fixes the issue you were seeing.

@gibfahn gibfahn closed this Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MylesBorins MylesBorins MylesBorins requested changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kasicka @gibfahn @MylesBorins @richardlau @nodejs-github-bot