nohoist baseline implementation #4979

connectdotz · 2017-11-22T04:21:20Z

Summary

this is the implementation of RFC #86 for #3882.

To prevent scope creep, this PR addressed only the issues directly impacting "install and run react-native in yarn workspaces with nohoist", which included:

install: including nohoist config, matching and flat tree generation. Many commands utilize the same tree generation, if there is no conflicting assumption, hopefully they should just work...
why: bug fixes and nohoist enhancement, it will now report all locations of the packages with the full dependency tree in one go. this can help self-diagnosis if nohoist went wrong.
add: replaced manual seeding with Manifest update and let install do the rest. The code change is not ideal as I tried not to change too much in this PR. Suggest later to refactor to make it cleaner, more like remove.js ...
also added many tests and fixtures

for more details please reference the RFC.

note:

lots of files in this PR, most of them are tests and fixtures.
I was not able to get a clean test run on my machine due to timeouts. Running the failed tests in isolation all passed, hopefully, CI will be fine...
nohoist did introduce a fundamental assumption shift in the system, which I've seen yarn commands yield unexpected results when operating in the nohoist workspace. I've only fixed those mentioned above, many might still exist. I would anticipate more follow-up PRs to address these issues as they come... maybe consider keeping nohoist in a separate branch or a beta channel to let it bake for a while...?

Test plan

for "react-native init"

create a yarn workspaces and add the following config to the root package.json

// project root package.json

"workspaces": {
   "packages": ["packages/*"],
    "nohoist": ["**/react-native", "**/react-native/**"]
}

then install react-native following the react-native instruction. It should just work....

you can also move the nohoist config to the workspace's package.json, if it is a private package:

// workspace package.json

"workspaces": {
    "nohoist": ["react-native", "react-native/**"]
}

it should produce the same result

for "create-react-native-app"

additional issues for this scenario:

create-react-native-app does not use yarn consistently in the workspaces setting, I submitted a PR (create-react-native-app/pull/503) to address this, hopefully, it will be merged in soon.
the react-native-scripts and Expo don't play well in workspaces, therefore, I had to add them to the nohoist list as well:

// project root package.json

"workspaces": {
    "packages": ["packages/*"],
    "nohoist": [
        "**/react-native", "**/react-native/**",
        "**/react-native-scripts", "**/react-native-scripts/**",
        "**/expo", "**/expo/**"
    ]

then you should be able to create and run the default expo app as documented.

- nohoist implementation - 'why' command fixes - 'add' command fixes - tests and test fixtures see [RFC yarnpkg#86](yarnpkg/rfcs#86) for detail

buildsize · 2017-11-22T04:24:28Z

This change will increase the build size from 10.41 MB to 10.44 MB, an increase of 30.99 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	901.44 KB	904.42 KB	2.98 KB (0%)
yarn-[version].js	3.92 MB	3.93 MB	10.68 KB (0%)
yarn-legacy-[version].js	4.07 MB	4.08 MB	11.55 KB (0%)
yarn-v[version].tar.gz	906.4 KB	909.99 KB	3.59 KB (0%)
yarn_[version]all.deb	669.99 KB	672.19 KB	2.2 KB (0%)

buildsize · 2017-11-22T04:24:28Z

This change will increase the build size from 10.26 MB to 10.28 MB, an increase of 24.17 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	886.52 KB	888.6 KB	2.08 KB (0%)
yarn-[version].js	3.86 MB	3.87 MB	8.68 KB (0%)
yarn-legacy-[version].js	4.01 MB	4.02 MB	9.48 KB (0%)
yarn-v[version].tar.gz	891.26 KB	893.28 KB	2.02 KB (0%)
yarn_[version]all.deb	667.97 KB	669.88 KB	1.92 KB (0%)

[PR #4979](yarnpkg/yarn#4979)

jmaher409 · 2017-11-22T20:56:22Z

@connectdotz thanks for making this change. This was a major issue for us with chromedriver and electron-chromedriver. Is there any timeline for this being merged to master?

achingbrain · 2017-12-04T10:52:47Z

This PR is exactly what my team needs - we're building a lerna/yarn workspaces-based monorepo that contains shared libraries, web apps and an electron app and so have some weird requirements around which packages should have some or all of their dependencies hoisted.

It'd be great if this feature could be merged.

1. added a new flags 'workspaces-nohoist-experimental' to disable nohoist. 2. added eligibility validation in Config.getWorkspaces, violation will be reported and config be ignored. 3. update test fixtures to add private flag for nohoist tests

connectdotz · 2017-12-08T13:05:02Z

@arcanis these are all the changes we discussed in RFC, the ball is in your court now...

* initial nohoist RFC * added exceptions to exclude linked packages from nohoist * addiing explanation for nohoist exception * updated nohoist matching and open question discussions * updated to reflect the initial implementation PR [PR #4979](yarnpkg/yarn#4979) * updated to reflect our discussion and code change

connectdotz · 2017-12-28T16:50:30Z

@arcanis, any progress on the review of this PR? is there something I should address to get this moving? we have been running with the local yarn build for almost a month, just wondering when we can use the official build...

burabure · 2018-01-03T14:49:08Z

sorry to drop in here, but just wanted to say we reeeeeeaaaaaaaaaaaaally need this. We have a super hacky patch now to avoid the issues but it's suboptimal at best.

connectdotz · 2018-01-07T19:59:00Z

ping @arcanis @bestander

bestander · 2018-01-07T20:00:11Z

Acknowledged. Will have a look asap

…

On Sun, Jan 7, 2018 at 11:59 AM ConnectDotz ***@***.***> wrote: ping @arcanis <https://github.com/arcanis> @bestander <https://github.com/bestander> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWFkDxs8_DZrflqrLaN0To0X1vPFUks5tISIOgaJpZM4Qm17D> .

bestander · 2018-01-08T07:39:28Z

__tests__/commands/add.js

@@ -42,20 +42,20 @@ const runAdd = buildRun.bind(
  },
 );

-test.concurrent('add without --ignore-workspace-root-check should fail on the workspace root', async () => {
-  await runInstall({}, 'simple-worktree', async (config, reporter): Promise<void> => {
+test.concurrent('add without --ignore-workspace-root-check should fail on the workspace root', (): Promise<void> => {


any reason for switching from async to promises?
Afaik async is used throughout the code and it would be better to keep it as is

bestander · 2018-01-08T07:42:27Z

__tests__/config.js

+
+class MockReporter extends BufferReporter {
+  lang = jest.fn();
+  // lang(key: LanguageKeys, ...args: Array<mixed>): string {


can be removed?

bestander · 2018-01-08T07:43:50Z

__tests__/package-hoister.js

@@ -311,3 +334,509 @@ test('will hoist packages under subdirectories when they cannot hoist to root',
  expect(result).toContainPackage('c@1.0.0', atPath('a', 'node_modules', 'c'));
  expect(result).toContainPackage('d@1.0.0', atPath('a', 'node_modules', 'd'));
 });
+
+describe('nohoist', () => {
+  function _showResults(results: HoistManifestTuples) {


function not used?

bestander · 2018-01-08T08:49:37Z

src/types.js

@@ -129,7 +134,7 @@ export type Manifest = {
  files?: Array<string>,
  main?: string,

-  workspaces?: Array<string>,
+  workspaces?: Array<string> | WorkspacesConfig,


any reason to keep the old structure?
Maybe use WorkspacesConfig for hoist cases too?

this is merely for backward compatibility so people who didn't need the new feature doesn't need to be forced to update their package.json... Internally the code is all converted to use WorkspaceConfig.

Ah, ok, let's keep it backwards compat, I missed that this is read from package.json

bestander · 2018-01-08T08:49:59Z

src/reporters/lang/en.js

@@ -183,7 +183,12 @@ const messages = {
    'Running this command will add the dependency to the workspace root rather than workspace itself, which might not be what you want - if you really meant it, make it explicit by running this command again with the -W flag (or --ignore-workspace-root-check).',
  workspacesRequirePrivateProjects: 'Workspaces can only be enabled in private projects',
  workspacesDisabled:
-    'Your project root defines workspaces but the feature is disabled in your Yarn config. Please check "workspaces-experimental" in your .yarnrc file.',
+    'Your project root defines wokkspaces but the feature is disabled in your Yarn config. Please check "workspaces-experimental" in your .yarnrc file.',


bestander · 2018-01-08T08:58:38Z

src/config.js

@@ -321,6 +322,7 @@ export default class Config {
      this._cacheRootFolder = String(cacheRootFolder);
    }
    this.workspacesEnabled = this.getOption('workspaces-experimental') !== false;
+    this.workspacesNohoistEnabled = this.getOption('workspaces-nohoist-experimental') !== false;


After thinking about this I think we don't really need to hide this feature behind a flag.
Workspaces is still behind "experimental" flag :)

I am happy to remove it but it was requested specifically by @arcanis cited workspaces-experimental will be removed soon, just want to make sure we are all on the same page...

ok, let's keep it

bestander

@connectdotz, thank you for writing this diff, this is a huge help for the React Native community.
Also loved the tests!

The PR is huge, so no surprise it took so long for us to have a look at it :)
I think it is great and good to go unless someone objects

Could you look at a couple of nits first?

bestander · 2018-01-28T16:36:53Z

All is fine, thanks for this work over the 3 months, @connectdotz, huge change!

bestander · 2018-01-28T16:37:43Z

Want to write a blog post about this feature https://yarnpkg.com/blog/?

MrLoh · 2018-01-28T16:43:31Z

Awesom that this has landed. Thanks for all the work. What does that mean now, when will it become available?

bestander · 2018-01-28T16:50:01Z

If you want to start using it now then feel free to use this bundle https://9293-49970642-gh.circle-artifacts.com/0/yarnpkg/yarn-1.4.1-20180128.1637.js

Otherwise we'll need to wait for 1.4.2, it should take a week or so for the next official release, depends on the core team workload

connectdotz · 2018-01-29T11:30:42Z

@bestander it's great to see this finally in! 👍

Want to write a blog post about this feature https://yarnpkg.com/blog/?

ok, will do. Will also publish the react-native repository we used to test nohoist, so there is a working example/demo and people can play with it themselves...

…readdir_files * upstream/master: (34 commits) feat(upgrade, add): Separately log added/upgraded dependencies (yarnpkg#5227) feat(publish): Publish command uses publishConfig.access in package.json (yarnpkg#5290) fix(CLI): Use process exit instead of exitCode for node < 4 (yarnpkg#5291) feat(cli): error on missing workspace directory (yarnpkg#5206) (yarnpkg#5222) feat: better error when package is not found (yarnpkg#5213) Allow scoped package as alias source (yarnpkg#5229) fix(cli): Use correct directory for upgrade-interactive (yarnpkg#5272) nohoist baseline implementation (yarnpkg#4979) 1.4.1 1.4.0 Show current version, when new version is not supplied on "yarn publish" (yarnpkg#4947) fix(install): use node-gyp from homebrew npm (yarnpkg#4994) Fix transient symlinks overriding direct ones v2 (yarnpkg#5016) fix(auth): Fixes authentication conditions and logic with registries (yarnpkg#5216) chore(package): move devDeps to appropriate place (yarnpkg#5166) fix(resolution) Eliminate "missing peerDep" warning when dep exists at root level. (yarnpkg#5088) fix(cli): improve guessing of package names that contain a dot (yarnpkg#5102) (yarnpkg#5135) feat(cli): include notice with license when generating disclaimer (yarnpkg#5072) (yarnpkg#5111) feat(cli): group by license in licenses list (yarnpkg#5074) (yarnpkg#5110) feat(cli): improve error message when file resolver can't find file (yarnpkg#5134) (yarnpkg#5145) ...

k15a · 2018-02-09T15:22:24Z

@bestander Hey, I don't want to be annoying but as it seems that many people including myself would love to use this feature officially I would like to know if there are any plans for a next release?

Thank you for the hard work on this feature and Yarn!

arcanis · 2018-02-09T16:36:20Z

You can use this feature right now by installing a nightly build. That would actually help us making sure everything is ready for prime time! :)

And the official release shouldn't be far, I just haven't took the time to make a decent changelog yet. By the end of next week I'd say.

sample-usr · 2018-02-13T09:46:06Z

@connectdotz awesome work!!
I'm testing this with the nightly in a yarn workspace managed by lernajs. I think we need to open an issue there as well to make use of this? Because currently I'm getting this error on running lerna bootstrap

TypeError: packageConfigs.some is not a function

I'm guessing this has to do with workspaces now being an object instead of an array.

k15a · 2018-02-13T09:48:16Z

@razagill This is already merged to Lerna in lerna/lerna#1254. Might not be released as well.

sample-usr · 2018-02-13T09:51:21Z

@k15a nice!

darthtrevino · 2018-02-15T23:26:37Z

Edit: I had a problem, but I was using the feature wrong. Thanks for the PR!

connectdotz · 2018-02-16T10:16:23Z

Glad people are trying nohoist. My bad that I should have published the examples and a more comprehensive introduction earlier (was waiting for 1.4.2 official release...), I think they would have spared some of your troubles... Here they are:

yarn-nohoist-examples, you should be able to run these with the nightly build or compare with your own projects. Many of your use cases should be covered there.
nohoist in workspaces, a more detailed introduction of nohoist, good for conceptual understanding and the inquisitive minds.

relferreira · 2018-02-19T16:30:52Z

I've been using the bundle yarn-1.4.1-20180128.1637.js for a while, and it's working pretty well. Thanks @connectdotz

dannycochran · 2018-02-23T02:12:51Z

@arcanis @connectdotz above it was mentioned this might get into 1.4.2, is that still the case? Is there a known timeline for 1.4.2?

arcanis · 2018-02-23T02:15:39Z

Yep, I planned to review a few PR and release the next version tomorrow! In the meantime, feel free to use the nightly, it would help us making sure everything is a-ok!

…

On Fri, Feb 23, 2018, 2:13 AM Danny Cochran ***@***.***> wrote: @arcanis <https://github.com/arcanis> @connectdotz <https://github.com/connectdotz> above it was mentioned this might get into 1.4.2, is that still the case? Is there a known timeline for 1.4.2? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_Wa5-K5W303LurwP6jXTE2DYH7kE1-ks5tXh6xgaJpZM4Qm17D> .

k15a · 2018-02-23T10:28:50Z

@arcanis I've tested our projects with the nightly build and everything works great. The only thing is that it takes a lot of time to install if you don't hoist anything. Maybe there is something Yarn could cache in the future or even have an option which hoists the dependencies but links them back.

Thanks for this great feature.

arcanis · 2018-02-23T15:57:42Z

1.5.0 is here! Thanks again for your hard work, @connectdotz 🎉

natew · 2018-03-10T02:33:35Z

Want to voice thanks for implementing this as well as note my initial install time was a bit high as well. For a medium(ish?) monorepo:

Another thing I've noticed is I don't see any yarn.lock files generated in my sub-folders, is this normal?

connectdotz · 2018-03-10T03:48:45Z

hi,

A few things you can help to isolate/improve the perf issue:

compare the performance without nohoist. it will help us isolate if the slowness is specifically caused by nohoist.
try yarn install with --link-duplicates and compare the perf. The flag instructs yarn to hard-link, instead of copying, duplicate files. Not only it reduces the footprint of your repo, I wonder if it has some perf advantage as well, if the slowness is caused by file-system related ops (?)... However, there was a bug that just got fixed(fix linkDuplicates for symlinks #5442), you will need to use the nightly build to avoid that issue.

We have to wait until yarnpkg/yarn#4979 is merged, as electron-builder can't resolve the dependencies otherwise.

connectdotz added 2 commits November 21, 2017 21:38

nohoist impl check point

7ad158d

- nohoist implementation - 'why' command fixes - 'add' command fixes - tests and test fixtures see [RFC yarnpkg#86](yarnpkg/rfcs#86) for detail

sync with remote

a25b483

connectdotz added a commit to connectdotz/rfcs that referenced this pull request Nov 22, 2017

updated to reflect the initial implementation PR

5c54ea5

[PR #4979](yarnpkg/yarn#4979)

connectdotz mentioned this pull request Nov 24, 2017

nohoist RFC yarnpkg/rfcs#86

Merged

arcanis self-assigned this Nov 24, 2017

connectdotz added 5 commits December 6, 2017 08:26

fix not adding devDependencies

c29ff68

add nohoist flag and eligibility check

499db53

1. added a new flags 'workspaces-nohoist-experimental' to disable nohoist. 2. added eligibility validation in Config.getWorkspaces, violation will be reported and config be ignored. 3. update test fixtures to add private flag for nohoist tests

revert path separator to '#' for display

d4676f4

pass through private flag in root manifest

7e3d407

fix lint error

edf4eec

bestander reviewed Jan 8, 2018

View reviewed changes

bestander approved these changes Jan 8, 2018

View reviewed changes

connectdotz and others added 3 commits January 8, 2018 11:06

addressing @bestander review comments on 1/8

820b4c1

Merge branch 'master' into nohoist

e6dac64

fix merge conflict

a8afc57

bestander merged commit 4bddb3a into yarnpkg:master Jan 28, 2018

This was referenced Feb 9, 2018

Add support for new yarn workspaces config format facebook/create-react-app#4001

Merged

Add support for new yarn workspaces config format lerna/lerna#1254

Merged

connectdotz mentioned this pull request Feb 16, 2018

(blog) introducing nohoist yarnpkg/website#777

Merged

tehwalris mentioned this pull request Feb 19, 2018

bump jest version wmonk/create-react-app-typescript#255

Open

zvictor mentioned this pull request Mar 8, 2018

Hoisting Electron with Yarn workspaces breaks Electron #4070

Open

connectdotz deleted the nohoist branch March 10, 2018 03:34

risenforward added a commit to risenforward/howdyQ that referenced this pull request Dec 18, 2018

chore(build): Disable yarn workspaces again until nohoist is implemented

a1c56fd

We have to wait until yarnpkg/yarn#4979 is merged, as electron-builder can't resolve the dependencies otherwise.

cgxxv pushed a commit to cgxxv/graphql-playground that referenced this pull request Mar 25, 2022

chore(build): Disable yarn workspaces again until nohoist is implemented

f534c5d

We have to wait until yarnpkg/yarn#4979 is merged, as electron-builder can't resolve the dependencies otherwise.

nohoist baseline implementation #4979

nohoist baseline implementation #4979

Conversation

connectdotz commented Nov 22, 2017 • edited Loading

buildsize bot commented Nov 22, 2017 • edited Loading

buildsize bot commented Nov 22, 2017

jmaher409 commented Nov 22, 2017

achingbrain commented Dec 4, 2017

connectdotz commented Dec 8, 2017 • edited Loading

connectdotz commented Dec 28, 2017

burabure commented Jan 3, 2018

connectdotz commented Jan 7, 2018

bestander commented Jan 7, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestander left a comment

Choose a reason for hiding this comment

bestander commented Jan 28, 2018

bestander commented Jan 28, 2018

MrLoh commented Jan 28, 2018

bestander commented Jan 28, 2018

connectdotz commented Jan 29, 2018

k15a commented Feb 9, 2018

arcanis commented Feb 9, 2018 • edited Loading

sample-usr commented Feb 13, 2018

k15a commented Feb 13, 2018

sample-usr commented Feb 13, 2018 • edited Loading

darthtrevino commented Feb 15, 2018 • edited Loading

connectdotz commented Feb 16, 2018 • edited Loading

relferreira commented Feb 19, 2018

dannycochran commented Feb 23, 2018

arcanis commented Feb 23, 2018 via email

k15a commented Feb 23, 2018

arcanis commented Feb 23, 2018

natew commented Mar 10, 2018 • edited Loading

connectdotz commented Mar 10, 2018 • edited Loading

connectdotz commented Nov 22, 2017 •

edited

Loading

buildsize bot commented Nov 22, 2017 •

edited

Loading

connectdotz commented Dec 8, 2017 •

edited

Loading

arcanis commented Feb 9, 2018 •

edited

Loading

sample-usr commented Feb 13, 2018 •

edited

Loading

darthtrevino commented Feb 15, 2018 •

edited

Loading

connectdotz commented Feb 16, 2018 •

edited

Loading

natew commented Mar 10, 2018 •

edited

Loading

connectdotz commented Mar 10, 2018 •

edited

Loading