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.

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

upgrade npm to 3.3.10 #3599

Closed
wants to merge 3 commits into from
Closed

upgrade npm to 3.3.10 #3599

wants to merge 3 commits into from

Conversation

iarna
Copy link
Member

@iarna iarna commented Oct 30, 2015

Please find enclosed:

  1. An update to .gitignore such that the debug module used by one of npm's dependencies won't be ignored again. It's worth noting that if this module ever moves in the node_modules folder that this .gitignore exception will have to be updated.

  2. An update test-npm.sh to use the npm 3.3.10 test-node script for kicking off the tests. We also now call npm rebuild in order to link in the binaries associated with the regular dependencies back in, as some tests need them. This wasn't required previously due to a bug that was triggering rebuilds at inappropriate times.

  3. And npm@3.3.10 – the last version included in node was 3.3.6 and since then the following notable changes have been made:

    1. Switch deps back to the nested form found in npm@2– this was necessary to allow the npm bundled with node v8 to upgrade. We'll likely go back to flat deps in npm@3 someday, but have no immediate plans to do so at this time.
    2. Fixed a bug that effectively resulted in npm rebuild being called far more often than intended during builds. This also meant that failures during this extraneous rebuild phase could cause optional deps to abort the install, instead of simply failing that one module.
    3. Fixed a crasher that showed up when package names weren't set.
    4. npm shrinkwrap will now complete successfully even if optional dependencies are missing.
    5. If a shrinkwrap already has dev deps, don't throw them away when someone later runs npm install --save.

    (As always, the full details are available in our changelog)

r: @Fishrock123
r: @chrisdickinson

@mscdex mscdex added the npm Issues and PRs related to the npm client dependency or the npm registry. label Oct 30, 2015
@Fishrock123
Copy link
Contributor

Will this help #3601 at all?

@iarna
Copy link
Member Author

iarna commented Oct 30, 2015

node, the one we just compiled, is definitely in the PATH when run, because we're editing the PATH.

npm has its own expectations of what $NODE means that aren't compatible with './node' (which is what it's set to by the build), and that was interfering with the test run.

I'd love to continue refining this over time to make test-npm.sh less and less weird, and that may also means tweaking the test-npm Makefile target. But

@iarna
Copy link
Member Author

iarna commented Oct 30, 2015

Regarding #3601, I can't see how? I don't see how npm could cause it (or #3606 for that matter)

@Fishrock123
Copy link
Contributor

node, the one we just compiled, is definitely in the PATH when run, because we're editing the PATH.

Doh, forgot about that.

@iarna
Copy link
Member Author

iarna commented Nov 2, 2015

@silverwind Previously a relative entry was tried and that didn't work. If anyone can figure out how to do that, it would obviously be much preferred. And yes, someday npm's deps will be flatified and we'll have to update the .gitignore when we do that. To that end...

I'm going to update I have updated npm's release process to include moving away the .gitignore and making sure that git status doesn't report anything in npm's directory. That way when we do flatify npm's deps in the future, we won't be bitten by this again.

@Fishrock123
Copy link
Contributor

This works happily even without node or npm in the PATH. LGTM.

Fishrock123 pushed a commit that referenced this pull request Nov 2, 2015
PR-URL: #3599
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit that referenced this pull request Nov 2, 2015
PR-URL: #3599
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 pushed a commit that referenced this pull request Nov 2, 2015
PR-URL: #3599
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123
Copy link
Contributor

Thanks again, landed in 827ee49...507fc53 :)

@iarna
Copy link
Member Author

iarna commented Nov 2, 2015

My test protocol now involves setting my path to not include node or npm before running the tests, so we shouldn't reintroduce that kind of problem.

rvagg pushed a commit that referenced this pull request Nov 7, 2015
PR-URL: #3599
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg pushed a commit that referenced this pull request Nov 7, 2015
PR-URL: #3599
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
rvagg pushed a commit that referenced this pull request Nov 7, 2015
PR-URL: #3599
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
@rvagg rvagg mentioned this pull request Dec 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants