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

[node-modules] Fixes soft and hard link merging after hoisting #2382

Closed
wants to merge 2 commits into from

Conversation

larixer
Copy link
Member

@larixer larixer commented Jan 16, 2021

What's the problem this PR addresses?

In Berry we have two link kinds for dependencies:

  1. A soft link - a kind of dependency when the files are not controlled by Berry. The examples of the soft links are workspaces, portal: and link: dependencies.
  2. A hard link - a kind of dependency, when the files are controlled by Berry. The examples of the hard links are normal dependencies installed from the registry or from GitHub.

After hoisting two dependencies might end up having the same name but different link types. For example, the root workspace can have inner workspace with the name 'foo' and depend on the package with the name foo from the registry, both of them are dependencies of the root workspace, but they have different link types. NM linker had not always handled this situation correctly and often tried to merge soft and hard links, when hard link just need to take precedence and just overwrite the soft link.

The problem were initially spotted by Babel team here:
babel/babel#12583 (comment)
CC: @nicolo-ribaudo @merceyz

How did you fix it?

Now NM linker do not try to merge soft and hard links together, the hard link just overwrites the soft link in the resulting nm tree, this does not affect the traversal, the nm will still traverse children of both dependency link kinds.

Checklist

  • I have set the packages that need to be released for my changes to be effective.
  • I will check that all automated PR checks pass before the PR gets reviewed.

@larixer larixer marked this pull request as draft January 16, 2021 14:07
@larixer
Copy link
Member Author

larixer commented Jan 20, 2021

Going to open another PR which addresses the problem directly

@larixer larixer closed this Jan 20, 2021
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.

None yet

1 participant