Skip to content

npm update @6.14.8#9

Merged
aminya merged 2 commits intomasterfrom
npm-update
Aug 23, 2020
Merged

npm update @6.14.8#9
aminya merged 2 commits intomasterfrom
npm-update

Conversation

@aminya
Copy link
Copy Markdown
Member

@aminya aminya commented Aug 22, 2020

Description of the change

This updates npm to 6.14.8.

Related: atom-community/atom#102

Verification

The CI passes. npm versions are Node version agnostic, so it will work on Node version without changing the behavior.

Drawbacks

none

Release Notes

  • Update npm to 6.14.8

@aminya
Copy link
Copy Markdown
Member Author

aminya commented Aug 22, 2020

For some reason, the tests assume that node-gyp should exist in node_modules, but that is not the case
https://github.com/atom-ide-community/apm/blob/475d35f92b0e257bb0c3fdbdb8e302ee8fb0d2bb/spec/install-spec.coffee#L536-L541

@DeeDeeG
Copy link
Copy Markdown
Member

DeeDeeG commented Aug 22, 2020

For some reason, the tests assume that node-gyp should exist in node_modules, but that is not the case

We need to switch the test to construct the path with require.resolve(), regardless of anything else.

I did that as a WIP thing at one point in the upstream PR modifying that test, but I didn't commit it for some reason. I was COMPLETELY new to JS then, so I was afraid to make unrelated changes, but it would have been the smart choice in hindsight.


We do not need to bump the locked npm version here, it's only a patch level bump and consumers of this module don't get the package-lock.json of this package, and as you may have seen, our Atom pulls in npm@6.14.8 already as the npm sub-dependency for its bundled apm.

@aminya
Copy link
Copy Markdown
Member Author

aminya commented Aug 22, 2020

We need to switch the test to construct the path with require.resolve(), regardless of anything else.

require.resolve is not reliable. We want the tests to be self-contained, and we should remove the assumptions about the test environment.

@DeeDeeG
Copy link
Copy Markdown
Member

DeeDeeG commented Aug 22, 2020

require.resolve() is dynamic, as is the location of node-gyp. (Well, I see you added it to package.json, so it's in a fixed location now I guess.)

Upstream removed node-gyp from their package.json because it had been neglected to be bumped in sync with npm. atom#832

As long as we use a version range in sync with npm, and are comfortable keeping it in sync with npm always, then I think adding it back to package.json is fine.

Edit: putting it in devDependencies is pretty reasonable. It's one more (arguably unneeded) dependency to download, but it's not very large, so the stakes are low.

@DeeDeeG
Copy link
Copy Markdown
Member

DeeDeeG commented Aug 22, 2020

How is require.resolve() not reliable? Genuinely curious, because this is the first I'm hearing that. I'm willing to be convinced, but I do think it works.

Edit to add: Recall that package-lock.json records the exact dependency tree, including what dependencies are nested in other dependencies' node_modules folders. So require.resolve(), which is dynamically found at runtime, should not have a hard job to do here.

@aminya
Copy link
Copy Markdown
Member Author

aminya commented Aug 23, 2020

require.resolve is not reliable if the package is supposed to be a direct dependency. This is case, is just a simple test, and so it doesn't matter.

@aminya aminya merged commit d319588 into master Aug 23, 2020
@aminya
Copy link
Copy Markdown
Member Author

aminya commented Aug 23, 2020

Upstreamed atom#900

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants