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: nested relative link: protocal dependency symlinks #3605

Merged
merged 3 commits into from
Jun 26, 2017
Merged

Fix: nested relative link: protocal dependency symlinks #3605

merged 3 commits into from
Jun 26, 2017

Conversation

stipsan
Copy link
Contributor

@stipsan stipsan commented Jun 7, 2017

When a link:a dependency also contain a link:b the symlink to link:b is broken.
I'm not sure how to fix it yet as I've never looked at Yarn's sourcecode before but at least I got a proper test case that reproduces the problem to get the ball rolling 😄

Expected test output in this PR (until the bug is fixed):

 FAIL  __tests__/commands/install/integration.js (64.908s)
  ● resolves the symlinks of other symlinked packages relative to the package using the link: protocol

    Error: expect(received).toEqual(expected)

    Expected value to equal:
      "../a/b"
    Received:
      "../b"

Edit

Seems like file-resolver have a solution for this already so I've pushed a patch to link-resolver that use the same logic. Hopefully it solves the problem 😃

@stipsan stipsan changed the title Add test case for broken nested symlinks using link: protocol Fix: nested relative link: dependency symlinks Jun 7, 2017
@stipsan stipsan changed the title Fix: nested relative link: dependency symlinks Fix: nested relative link: protocal dependency symlinks Jun 7, 2017
@arcanis
Copy link
Member

arcanis commented Jun 8, 2017

I'm not sure I totally understand what you mean - the link: protocol just creates a symlink to a location, that's it. It doesn't install any nested dependency.

@stipsan
Copy link
Contributor Author

stipsan commented Jun 9, 2017

@arcanis if you look at the test case I added here you can observe that it does resolve dependencies of a link, and that behavior happens before applying my fix.

I thought this behavior was a feature? That's why I tried to fix the broken symlinks that result from nested dependencies 😄

'resolves the symlinks of other symlinked packages relative to the package using the link: protocol',
async () => {
await runInstall({}, 'install-link-nested', async (config): Promise<void> => {
const expectPath = path.join(config.cwd, 'node_modules', 'b');
Copy link
Member

Choose a reason for hiding this comment

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

So do I understand it right?

root
  -- > link: a --> link: b

Gets installed

node_modules
  a (symlink) -> ../a
  b (symlink) -> ../a/b

Copy link
Member

@bestander bestander Jun 16, 2017

Choose a reason for hiding this comment

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

I am worried that we touch what is inside a, probably a symlinked dependency should not participate in deduping?

And be like

node_modules/
  a (symlink) -> ../a

a/ 
  b/
  node_modules/
  b (symlink) -> ../b

I wonder if several projects will link to a, would they be modifying their contents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the following is what gets installed:

node_modules
  a (symlink) -> ../a
  b (symlink) -> ../a/b

This is however what is getting installed on develop currently:

node_modules
  a (symlink) -> ../a
  b (symlink) -> ../b

And that means the b symlink is going nowhere. I can't comment on deduping and that behavior as I'm new to the yarn codebase and have only basic understanding on resolvers in general and ExoticResolver logic specifically 😕

I can dig into it though and add a test case regarding:

I wonder if several projects will link to a, would they be modifying their contents

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, looks like it is fine then.
We'll probably be discovering more issues with transitive link: and file: dependencies, we'll deal with them as they come.
Feel free to investigate a bit more.

@bestander bestander merged commit a7e6efe into yarnpkg:master Jun 26, 2017
@bestander
Copy link
Member

Thanks, @stipsan, good fix!

@stipsan stipsan deleted the relative-link-dependencies branch July 24, 2017 08:59
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

Successfully merging this pull request may close these issues.

3 participants