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

Regression: 0.23.2, yarn uses npm registry for local file dependencies #3187

Closed
sergejsha opened this issue Apr 19, 2017 · 24 comments
Closed

Comments

@sergejsha
Copy link

sergejsha commented Apr 19, 2017

Bug-Report
yarn v0.23.2
macOs v10.12.4
nodejs v6.9.4

Current behavior:
In the main component I have a dependency on a local component as following:

"dependencies": {
       ...
        "module-model": "file:../module-model"
}

I increase the version of the local module-model component and run yarn install on the main component. Yarn says Already up-to-date which is wrong. After trying yarn upgrade-interactive and choosing the component

screen shot 2017-04-19 at 11 00 19

yarn says that the local component cannot be found on the "npm" registry, which is incorrect behavior.

screen shot 2017-04-19 at 11 00 32

Instead of checking "npm" registry yarn has to check local directory.

Expected behavior
When yarn install is executed, yarn shall check the local dependency component version and if its version differs from the version of the component stored in node_modules directory, yarn shall update node_modules with the new version of the local component.

p.s. As of now the only way to get component updated is to call yarn add "file:../module-model"

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

It seems there's two separate issues here. The first one is yarn reporting that your package installation is Already up-to-date, and the second one is that upgrade-interactive only works with registry packages (duplicate of #2922).

@tibdex
Copy link

tibdex commented Apr 19, 2017

Hi, just wanted to add that the issue also appears on a fresh install with the yarn cache cleaned and no node_modules folder yet.

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

If that's so, it's a separate issue. I've tried with the following script and it worked fine:

YARN=yarn

rm -rf /tmp/yarntest
mkdir /tmp/yarntest

mkdir /tmp/yarntest/a
echo '{ "name": "a", "version": "1.0.0", "dependencies": { "b": "file:../b" } }' > /tmp/yarntest/a/package.json

mkdir /tmp/yarntest/b
echo '{ "name": "b", "version": "1.0.0" }' > /tmp/yarntest/b/package.json

cd /tmp/yarntest/a

"$YARN" install

cat yarn.lock
ls -l node_modules/b
cat node_modules/b/package.json

Can you reproduce it by running it?

@sergejsha
Copy link
Author

sergejsha commented Apr 19, 2017

@arcanis First yarn install on the main component (component a) without node_modules directory works just fine. Thus the scripts you wrote will also work fine. The issue happens only when you subsequently change the version of component b and run yarn install on the component a again. The content of node_modules of the component a stays unchanged and yarn reports "already uptodate", which is not the expected behavior.

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

Yep, I've been able to reproduce this, but not was @tibdex reported

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

The main issue is that we try to be smart about install, and to skip it when we notice that nothing changed in the yarn.lock and the node_modules directory (because it then means that the two of them are in sync). The file: protocol (and a few others, such as installing a remote branch) break this assumption, because the content they refer to might change even if the lockfile doesn't.

Not sure what would be the best course of action here. We could always run an install on "volatile" dependencies, but that would have a cost in term of performances :/

@tibdex
Copy link

tibdex commented Apr 19, 2017

Your script works fine indeed @arcanis but it fails when you have:

// /tmp/yarntest/a/package.json
{
  "name": "a",
  "version": "1.0.0",
  "dependencies": {
    "b": "file:../b-1.0.0.tgz"
  }
}

where b-1.0.0.tgz has been created by running npm pack in the /tmp/yarntest/b directory.

The error is:

yarn install v0.23.2
info No lockfile found.
warning a@1.0.0: No license field
[1/4] Resolving packages...
error An unexpected error occurred: "https://registry.yarnpkg.com/b-1.0.0.tgz: Request failed \"404 Not Found\"".

@sergejsha
Copy link
Author

sergejsha commented Apr 19, 2017

@arcanis

I see. I would expect yarn install --force to re-checks the versions of file: and the other remote branches and update the lockfile (and node_modules) correspondingly. yarn install can stay the way it is now. This will keep yarn fast and give us an additional option to force a longer (but needed) update. What do you think?

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

@tibdex That seems like a separate issue; would you mind opening a new issue to keep discussions in the correct order? 😃

@beworker Then good news, it should already work as you described! :D Running install with the --force flag will basically erase the node_modules folder and install everything all over again. In the future it might be interesting to add an option to only update volatile packages.

Testcase:

YARN=yarn

rm -rf /tmp/yarntest
mkdir /tmp/yarntest

mkdir /tmp/yarntest/a
echo '{ "name": "a", "version": "1.0.0", "dependencies": { "b": "file:../b" } }' > /tmp/yarntest/a/package.json

mkdir /tmp/yarntest/b
echo '{ "name": "b", "version": "1.0.0" }' > /tmp/yarntest/b/package.json

cd /tmp/yarntest/a

"$YARN" install

cat yarn.lock
ls -l node_modules/b
cat node_modules/b/package.json

echo '{ "name": "b", "version": "2.0.0" }' > /tmp/yarntest/b/package.json

"$YARN" install

# Won't be updated because the install has been early aborted by the initial check
cat yarn.lock
ls -l node_modules/b
cat node_modules/b/package.json

"$YARN" install --force

# Will be updated because everything has been force-updated
cat yarn.lock
ls -l node_modules/b
cat node_modules/b/package.json

@sergejsha
Copy link
Author

Hmm... You are right. I believe I tried -force option with a single dash, which is why it didn't work for me. Just tried again with --force and it worked. That's not the issue anymore. Thanks for supporting!

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

Glad it worked 👍

I'm going to close this issue in favor of #2922 to track the upgrade-interactive behavior, then.

@arcanis arcanis closed this as completed Apr 19, 2017
@wclr
Copy link
Contributor

wclr commented Apr 19, 2017

So I don't get, for me it doesnt'' work in 0.22.0 with --force option too?

@sergejsha
Copy link
Author

@whitecolor This must be the version which didn't work for me too, because I clearly remember I tried --force and the dependency has not been updated. You should update to the latest 0.23.2 and it will work.

@tibdex
Copy link

tibdex commented Apr 19, 2017

@arcanis, I commented on #3168 for the .tgz issue.

@wclr
Copy link
Contributor

wclr commented Apr 19, 2017

You should update to the latest 0.23.2 and it will work.

I have just switched to stable-0.23 branch, it doesnt' work isn't it not enough?

@sergejsha
Copy link
Author

sergejsha commented Apr 19, 2017

@arcanis just wanted to mention that it would be cool having an option for forcing just volatile packages to be reinstalled. For simplicity I would call it yarn install --please ;)

@wclr
Copy link
Contributor

wclr commented Apr 19, 2017

@arcanis should it work on stable-0.23 branch or not?

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

Yes, it should. If it doesn't, can you please put together a small script I can use to reproduce this issue? Like the one above?

@wclr
Copy link
Contributor

wclr commented Apr 19, 2017

mkdir "test"
cd test
yarn init
mkdir my-local-package
cd my-local-package
yarn init
cd ..
yarn add file:my-local-package
touch my-local-package/change.txt
yarn upgrade my-local-package
# yarn --force
# yarn add file:my-local-package

check test/node_modules/my-loca-package it should contain change.txt, but my doesn't

@arcanis
Copy link
Member

arcanis commented Apr 19, 2017

Thanks! I've been able to reproduce it, but not 100% of the time. It seems there's something fishy. Possibly a race condition somewhere. I'm gonna reopen this issue and investigate more.

@CrabDude
Copy link
Contributor

For local file: caching see #2165

@wclr
Copy link
Contributor

wclr commented Apr 30, 2017

@arcanis has it been fixed? How is it going to work?

  1. will be cached involved at all for file: deps?
  2. will be file: deps upgraded (copied to node_modules and deps checked) on just yarn (not yarn --force)?

@bestander
Copy link
Member

Yes, this was fixed now.
Afaik it creates a new entry in cache every time you install a file: dependency.
Alternatively you can use link: specifier that creates symlinks instead of copying files

@wclr
Copy link
Contributor

wclr commented Jun 29, 2017

Oh I didn't know link: is implemented. This is good. This thing npm/npm#15900 caused a lot of undesirible behaviour with npm 5 that decided to treat file: as symlinks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants