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

feat(node-modules): Portal support #2481

Merged
merged 18 commits into from
Mar 29, 2021
Merged

feat(node-modules): Portal support #2481

merged 18 commits into from
Mar 29, 2021

Conversation

larixer
Copy link
Member

@larixer larixer commented Feb 15, 2021

What's the problem this PR addresses?

This PR adds support for portals to the node-modules linker. The application that uses portals must be started with --preserve-symlinks Node option. Only those portals that point inside project directory or have no conflicting dependencies with their parent package are representable in node_modules installs and are supported. When conflict is found, the nm linker outputs error and abandons install.

How did you fix it?

Portal dependencies are now added to the dependency graph before hoisting. Conflicts in portal dependencies are detected and reported.

This PR also fixes #2266

CC: @andreialecu

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 requested a review from arcanis as a code owner February 15, 2021 15:13
@larixer larixer marked this pull request as draft February 15, 2021 15:13
larixer and others added 4 commits February 15, 2021 20:18
Co-authored-by: Andrei Alecu <andreialecu@users.noreply.github.com>
Co-authored-by: Andrei Alecu <andreialecu@users.noreply.github.com>
@larixer larixer marked this pull request as ready for review February 16, 2021 14:38
@arcanis
Copy link
Member

arcanis commented Feb 24, 2021

Would it be worth to have the linker print a warning explaining that --preserve-symlinks will be required for the install to properly behave? Users will be able to filter it out with logFilters if they so wish, but without it I'm worried they would try running the program as usual, see it fail, and think it's a bug.

@larixer
Copy link
Member Author

larixer commented Feb 24, 2021

Yes, I think, it makes sense, I will add the warning, when any of portal targets has at least one dependency (otherwise the --preserve-symlinks are not needed, if all portal targets have zero dependencies)

@andreialecu
Copy link
Contributor

This is currently still missing the workspace protocol support, correct?

@larixer
Copy link
Member Author

larixer commented Feb 25, 2021

@andreialecu I'm not sure what do you mean, the workspace protocol had always been fully supported by nm linker

@merceyz
Copy link
Member

merceyz commented Feb 25, 2021

This is currently still missing the workspace protocol support, correct?

The core doesn't resolve the workspace protocol from external projects so it's unrelated to this PR

@andreialecu
Copy link
Contributor

@larixer sorry for not being clear enough, I'm referring to the discussion on Discord here: https://discord.com/channels/226791405589233664/226793713722982400/810902897947836466

@larixer larixer force-pushed the larixer/nm-portals branch from e41d66a to d37d1d5 Compare March 11, 2021 11:14
larixer and others added 3 commits March 12, 2021 09:08
* fix: restore build state to prevent rebuilds

* fix: persist build state when lockfile doesn't match persisted resolutions
@larixer larixer force-pushed the larixer/nm-portals branch from 66754db to 00dcb34 Compare March 20, 2021 10:03
@larixer larixer marked this pull request as draft March 20, 2021 11:14
@larixer
Copy link
Member Author

larixer commented Mar 20, 2021

Switched to a draft, because apparently if portal target is inside the project and it is used more than once, the portal must be fully hoistable too, otherwise multiple hoisting layouts will arise for such a portal which will lead to broken install. Going to implement this extra constraint here

@larixer larixer marked this pull request as ready for review March 26, 2021 07:37
@larixer
Copy link
Member Author

larixer commented Mar 26, 2021

Actually in the case above the inner portal MUST NOT be fully hoistable, it must have SINGLE hoisting layout. Hence, this PR is okay. There are two other problems that should be addressed in follow up PRs, but these problems existed for long time already and not directly related to portals:

  • The problem that soft links must have only SINGLE hoisting layout. After much thinking about this problem I came to conclusion that this needs to be implemented as strict addition to the current hoisting algorithm in a form of finding common hoisting layout satisfying all instances of a given SOFT link. The code of uniting hoisting layouts for a SOFT link currently exists in buildNodeModulesTree and is simply incomplete and hence incorrect, it must be transferred instead to the hoisting code, completed and unit tests should be added to cover it.
  • The other problem is tracing peer dependencies of a SOFT link. If SOFT link inherits dependency from the folder that is not PARENT on the file system, then --preserve-symlinks option must be used, otherwise it will not find peer dependency correctly. This peer tracing should be implemented in a separate PR. I believe this case appears seldom in practice, but Yarn should detect it and warn with diagnostic info.

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.

[Bug] portal protocol fails when there is executable in a read only directory
4 participants