Skip to content

fix nohoist binLink#5492

Merged
arcanis merged 4 commits intoyarnpkg:masterfrom
connectdotz:fixBinLinkNohoist
Mar 12, 2018
Merged

fix nohoist binLink#5492
arcanis merged 4 commits intoyarnpkg:masterfrom
connectdotz:fixBinLinkNohoist

Conversation

@connectdotz
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 ...)

Comment thread 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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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