File url normalization

[xandris]

Summary

When resolving dependencies against file: URLs that have relative paths, descendent modules' file: URLs were resolving against the CWD instead of against package.json.

I'm not claiming that depending on packages in other directories is a good idea, but this is a use case that npm seems to support but yarn doesn't. It seems like it should work but doesn't.

Test plan

Given a set of modules:

.
├── package.json
├── root-a
│   ├── index.js
│   └── package.json
├── root-b
│   └── package.json
└── sub
    └── sub-a
        └── package.json

The dependencies go like this: . -> sub-a -> root-b -> root-a.

Root calls sub-a file:sub/sub-a.

sub-a calls root-b file:../../root-b

root-b calls root-a'file:../root-a`

Saying yarn correctly resolves sub-a, but then looks up two directories from CWD to try to find root-b and fails:

yarn install v0.16.1
info No lockfile found.
warning No license field
[1/4] 🔍  Resolving packages...
error "/Users/aparker/src/yarn/__tests__/fixtures/root-b" doesn't exist.
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

This pull requests attempts to fix that by normalizing the in-memory package.json to resolve all file: URLs upon reading a package.json and relativizing them against CWD. I'm not sure if this is the correct approach but it seems to work.

Another approach I considered was attaching the current manifest to PackageRequest during resolution and retrieving that info in FileResolver, using the base path of the manifest, if any, instead of this.config.cwd.

[hawkrives]

This should fix #815, I think (and, by extension, #1263).

[sebmck]

normalize-manifest isn't really the right place for this kind of thing. Ideally we should be doing the normalisation here.

[xandris]

@kittens I tried that approach too and it worked. The only weird thing was the original relative paths ended up in yarn.lock. I just started poking around the codebase yesterday so I don't know if that's an issue.

[ghost]

(Edit: was typing while previous comment appeared, anyway, it explains it as-well)

@kittens to add on your comment: do you mean normalizing only the loc on resolve before reading the manifest?

If so, I think it would show a side-issue (that this PR solves), that is: pattern names are a concatenation of dependency properties from the manifest. This would lead to successful install but a misleading yarn.lock with confusing paths.

Given this example, yarn.lock would contain:

• root-b@file:../../root-b, while it should be root-b@file:./root-b I believe

Doing the normalization later on the .dependencies after reading the manifest in file-resolver would maybe then be the best of two worlds ✨ ?

@xandris

While checking this out I noticed two small things (epic otherwise!):

• I think you are missing optionalDependencies to be fixed?
• There is a fancy removePrefix method in existance

[xandris]

@verth @kittens yeah I forgot about optionalDependencies since I've never used them 😖

How should I proceed? Should I do the same thing, but in FileResolver to the manifest it reads?

const manifest = await this.config.readManifest(loc, this.registry);

[xandris]

I can't get it to work like that. It seems to work from the command line, but the unit test fails. I can't tell what's going on; it seems to run the yarn install command, and that works, but then it takes a second pass that has wrong paths. Help... what are the tests doing?

[xandris]

I think the code that reads the manifest is caching it, so the same manifest is getting normalized by FileResolver multiple times... How should I handle this?

[xandris]

I could add a WeakSet that tracks which manifests have already been normalized... That lets the test pass

[ghost]

Made a quick implementation with your tests, based on the the manifest normalizing as in your PR, and had green tests, could it maybe be something small you missed? I made a Pull Request to my own fork so you can easily see the diff, and maybe find out what could have gone wrong?

Hope it will help you on the way!

[xandris]

Yes, that helped, thank you! Submitted for your approval, the PR now copies the manifest if and only if any of the paths were actually normalized.

[ozsay]

what is the status of this PR ? it fixes our local dependencies issues. we can't migrate to yarn without it.

Thanks

[justin808]

@bestander Any idea when this will be shipped? Some projects are blocked by this.

[bestander]

@justin808 it is already in 0.18.0