Ref #2165 - file-resolver should invalidate cache with a new hash

[mtraynham]

Summary
When using a file: resolution for dependencies, the dependency is always being pulled from yarn's cache; the first time being the exception, which puts it into the cache.

Currently the cached package will only have the name and version, like the following:

ls `yarn cache dir` | grep npm-foo
# npm-foo-0.0.1

This change ensures that the compared dependency always generates a new hashed name which will invalidate the cache. The new hash name looks like:

`npm-${package.name}-${package.version}-${uuid.v4()}-${(new Date()).getTime()}`

I've found two small caveats which are worth noting.

1. Once a dependency is installed to the node_modules directory, the hash comparison doesn't have any effect when running yarn install is called again. Only package.json version differences will have an effect, which are represented with differences in the yarn.lock file. This can be circumvented with yarn install --force, which invalidates the local package and replaces it with a new copy, even if it is the same version.

Personally, I think this is correct functionality.

# Package added to node_modules and yarn cache
yarn install
# No effect (lock file is the same, nothing added to cache)
yarn install
# Invalidates cache and overrwrites dependency in node_modules
yarn install --force

2. Any install where the local package is not present in node_modules or using the install --force option, will add a duplicate entry to the yarn cache. The takeaway here is that we shouldn't cache the dependency at all as suggested here, but I'm probably not the right person to make that change.

Test plan

1. Create two local projects, both with a package.json file and index.js file. One package depends on the other using a file: reference.

mkdir -p foo
echo "{\"name\":\"foo\", \"version\": \"0.0.1\",\"main\":\"index.js\"}" > foo/package.json
touch foo/index.js
mkdir -p bar
echo "{\"name\":\"bar\", \"version\": \"0.0.1\",\"main\":\"index.js\",\"dependencies\": {\"foo\":\"file:../foo\"}}" > bar/package.json
touch bar/index.js

2. Run yarn install on the bar package

cd bar
yarn install
cd ..

3. Validate the named hash entry in the cache

ls `yarn cache dir` | grep npm-foo
# npm-foo-0.0.1-4997b76e-1d5c-4dfa-a001-5d2424c390dc-1488908398088

4. Change the index file of foo, remove node_modules of bar and reinstall.

echo "console.log('test');" > foo/index.js
cd bar
rm -r node_modules
yarn install

5. Check foo package index.js file for change

cat node_modules/foo/index.js
# console.log('test');

[mtraynham]

@bestander I know little to nothing about Jest and haven't used async/await much. The failing test is checking for a package in the cache with the path npm-dummy-0.0.0. That would have been moved as of this change to something like npm-dummy-0.0.0-1a938caa-0d85-403f-b381-5177c895c23a-1488920740358.

You can use node-glob to find it and get the first occurence, but maybe you have a better recommendation?

glob(`${path.join(config.cacheFolder, 'npm-dummy-0.0.0')}*`, (error, files) => files[0])

[mtraynham]

That particular test is fixed, but now getting a bunch of failures caused by 500's

Error: https://codeload.github.com/substack/node-mkdirp/tar.gz/f2003bbcffa80f8c9744579fabab1212fc84545a: Request failed "500 Internal Server Error"

[bestander]

Thanks for sending this PR.
I have skimmed through it, could you add a test as you have described in the Test Plan?
I am sure there is an open issue for this PR and IIRC someone already added a test that is disabled currently.

[mtraynham]

@bestander I've modified the existing test that was added in #2443, i.e. when not using --force the install retains the existing local package in node_modules. This test actually had a bug in it, where updating the child package file was not within the sandboxed test directory, but actually within the yarn __test__ directory. I've added a test for using --force which will do the opposite and reinstall the modified local package, which also denotes it missed the yarn cache.

On top of this, I had to correct the first test that broke ("properly find and save build artifacts") as it was failing intermittently. It initially broke because npm's install script would run and the new hash logic doubles the package in the cache. This is likely another reason to not cache local packages, but that is beyond this PR. Instead, I rewrote it so glob would find the correct cached build with the correct .yarn-metadata artifacts and added a TODO.

Let me know if there's anything else I need to do. Thanks!

[mtraynham]

I've rebased this to resolve any conflicts. The failures look like 502 Bad Gateway responses which seem unrelated. Any chance this is going to make the next release? Wondering if I need an alternate route for the time being.

[bestander]

Thanks, @mtraynham we are in an unstable CI phase right now, too many requests are getting timed out.

There is an RFC yarnpkg/rfcs#34 related to this feature.
Can you double check if your change is aligned with it?

[bestander]

I understand that npm developers seem to be unhappy about the copy behavior that npm@2 was doing

[mtraynham]

@bestander I believe this is definitely related, but I'm probably not the one to ask if file: and link: are aligned in scope. Seems like you guys have a lot of debate going on over there.

Per #2165, it seems like there is a lot of interest in just linking local dependencies with symlinks. Honestly, I don't have a use case for caching local dependencies at all. As it stands today, the file: protocol doesn't update the local package without a cache clean and nuking it from the node_modules directory. This PR is really just a temp fix, given there would be a new cache entry every time yarn install --force. For my own development, I imagine I would --force by default to always refresh for every build and clean the cache every so often to avoid bloat.

Is there a use case where you want to cache a local package and not have it updated? I'm probably not the one to make that justification, but if it were me, I'd say no. There are other mechanisms for getting a local dependency back to a different state, such as source control and it's local, so a copy is probably slower than a symlink. And if that's a no, link: does seem more appropriate.

[bestander]

Hey, @mtraynham, sorry for taking that long.
I think this is awesome and ready to merge as is.
Could you rebase please?

[mtraynham]

@bestander I've rebased, let me know if you need anything else. Thanks!

[bestander]

Looks like cache test got broken

FAIL tests/commands/cache.js (5.2s)
● clean
Error: expect(received).toEqual(expected)

Expected value to equal:
  2
Received:
  3

[mtraynham]

So this looks like a new test since the rebase. A count of 3 is expected and I'm going to increment that within the test and provide a comment.

Some clarification on the reason it is now 3, this test uses the file: protocol fixture and now that we've changed the hash scheme, it may not just be one local package, but 2 in the cache; 3 including the .tmp folder which is already mentioned with a comment.

[mtraynham]

Thanks!