-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Conversation
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
c1d5025
to
262c997
Compare
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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.
Ok, yeah, this should be fine. It's a patch. It'll be in 6.10.2. |
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