Skip to content

fix nohoist binLink #5492

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 12, 2018
Merged

fix nohoist binLink #5492

merged 4 commits into from
Mar 12, 2018

Conversation

@connectdotz
Copy link
Contributor

@connectdotz connectdotz commented Mar 9, 2018

Summary
address #5487

src

  • package-linker.js: fix the following:
  1. nohoist modules should have bin linked to its local module, not the one in PackageResolver's cache.
  2. if a module is in nohoist list, we should not create a bin link under root

test

  • bin-link.js and fixtures: added new tests and fixtures to test nohoist with duplicated bin links.

Test plan

existing and new tests should pass.

@buildsize
Copy link

buildsize bot commented Mar 9, 2018 •
edited
Loading

This change will increase the build size from 10.51 MB to 10.51 MB, an increase of 4.92 KB (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 910.69 KB 911.15 KB 475 bytes (0%)
yarn-[version].js 3.96 MB 3.96 MB 1.63 KB (0%)
yarn-legacy-[version].js 4.11 MB 4.11 MB 1.95 KB (0%)
yarn-v[version].tar.gz 916.08 KB 916.64 KB 581 bytes (0%)
yarn_[version]all.deb 676.31 KB 676.62 KB 316 bytes (0%)

@connectdotz
Copy link
Contributor Author

connectdotz commented Mar 10, 2018

the tests pass on linux/mac but failed on appVeyor, seems like a windows specific issue... I am trying to get a windows docker container running on my mac so I can debug it. Not a lot of experience with windows-docker, not sure if that is the best way to go about this.... maybe there is a way to debug on appVeyor session directly? Anybody has done similar things before? @bestander @arcanis do you have any suggestion?

@connectdotz
Copy link
Contributor Author

connectdotz commented Mar 10, 2018 •
edited
Loading

never mind, I found the problem. Had to use my dad's pc, that was painful... will send updates tomorrow

@connectdotz
Copy link
Contributor Author

connectdotz commented Mar 11, 2018

alright, we are back to the business now... hopefully, we will get a reviewer soon...

arcanis
arcanis reviewed Mar 11, 2018
View reviewed changes
Copy link
Member

@arcanis arcanis left a comment •
edited
Loading

Choose a reason for hiding this comment

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

(forgot to press the 'submit review' button ...)

src/package-linker.js

if (isDirectRequire || (this.topLevelBinLinking && !linksToCreate.has(name))) {
// a nohoist package should not be linked at topLevel bin
if (!isNohoist && (isDirectRequire || (this.topLevelBinLinking && !linksToCreate.has(name)))) {
Copy link
Member

@arcanis arcanis Mar 9, 2018

Choose a reason for hiding this comment

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

What will happen if a toplevel dependency is marked as nohoist? 🤔

Copy link
Contributor Author

@connectdotz connectdotz Mar 11, 2018

Choose a reason for hiding this comment

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

very thoughtful question, @arcanis! 👍

The top-level packages should never be marked nohoist, see here. I shall add some tests to ensure this is indeed the case.

@connectdotz
Copy link
Contributor Author

connectdotz commented Mar 12, 2018

@arcanis new tests are in, let me know if you have other question.

arcanis
arcanis approved these changes Mar 12, 2018
View reviewed changes
Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @connectdotz for maintaining this feature :)

@arcanis arcanis merged commit 6601f4b into yarnpkg:master Mar 12, 2018
@connectdotz connectdotz deleted the fixBinLinkNohoist branch March 17, 2018 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@arcanis arcanis arcanis approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@connectdotz @arcanis