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

Fix npm ci with file: dependencies #216

Closed
wants to merge 1 commit into from

Conversation

jfirebaugh
Copy link
Contributor

Partially reverts #40/#86, keeping the "Don't record linked deps as bundled" part but reverting the "Don't iterate into linked deps" part. It seems that we need to record dependencies of linked deps in order for npm ci to work.

Fixes https://npm.community/t/6-8-0-npm-ci-fails-with-local-dependency/5385
Fixes https://npm.community/t/npm-ci-fail-to-local-packages/6076

@jfirebaugh jfirebaugh requested a review from a team as a code owner July 19, 2019 01:12
Partially reverts npm#40/npm#86, keeping the "Don't record linked deps as bundled" part but reverting the "Don't iterate into linked deps" part. It seems that we need to record dependencies of linked deps in order for `npm ci` to work.

Fixes https://npm.community/t/6-8-0-npm-ci-fails-with-local-dependency/5385
Fixes https://npm.community/t/npm-ci-fail-to-local-packages/6076
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

This patches up the problem that's incorrectly making npm ci blow up, but I think it's still going to have to be blown away for npm v7.

The problem (which I think is definitely out of scope for this PR) is illustrated nicely in the https://github.com/jfirebaugh/npm-ci-bug reproduction.

With this patch applied, we see that left-pad was installed twice:

$ find . -name left-pad -o -name scratch
./submodule/node_modules/left-pad
./submodule/node_modules/scratch
./node_modules/left-pad

But, note that scratch is a symbolic link, so only the ./node_modules/left-pad will ever be reached. When the code in ./submodule/node_modules/scratch/... runs require('left-pad'), it'll load from the realpath, not from the path it was loaded at.

This patch makes npm ci account for npm's failure to interpret this tree properly, which is a net good.

The only thing making me wonder whether to land it is whether this might change the contents of package-lock.json, which we try not to do in a minor/patch version. It seems like, since it only would affect npm ci, and only in cases where npm ci currently explodes, it should be fine as a patch?


var target = path.resolve(pkg, '../local-dependency')

test('setup', function (t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to make or remove the common.pkg folder any more. (In fact, doing so can throw off the root ownership testing.) Both the common.cache and common.pkg are managed by the common-tap.js module.

Just write the package.json file. The rest is taken care of :)

name: 'local-dependency',
version: '0.0.0',
dependencies: {
underscore: '1.5.1'
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I just realized, this is going to hit the public registry during a test. I'll switch it to use the mock registry, but for future reference, that's usually not a great idea. With as many tests as npm has, if it had to look further than localhost, it's kind of a nightmare.

@isaacs isaacs added semver:patch semver patch level for changes and removed needs-discussion labels Jul 21, 2019
@isaacs
Copy link
Contributor

isaacs commented Jul 21, 2019

Ok, yeah, this should be fine. It's a patch. It'll be in 6.10.2.

@isaacs isaacs closed this in 2fb0509 Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants