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

nohoist baseline implementation #4979

Merged
merged 16 commits into from Jan 28, 2018
Merged

nohoist baseline implementation #4979

merged 16 commits into from Jan 28, 2018

Conversation

connectdotz
Copy link
Contributor

@connectdotz connectdotz commented Nov 22, 2017

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:

  1. 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.
  2. 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
Copy link

buildsize bot commented Nov 22, 2017

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
Copy link

buildsize bot commented Nov 22, 2017

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%)

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

@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?

@arcanis arcanis self-assigned this Nov 24, 2017
@achingbrain
Copy link

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
Copy link
Contributor Author

connectdotz commented Dec 8, 2017

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

arcanis pushed a commit to yarnpkg/rfcs that referenced this pull request Dec 8, 2017
* 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
Copy link
Contributor Author

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

burabure commented Jan 3, 2018

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
Copy link
Contributor Author

ping @arcanis @bestander

@bestander
Copy link
Member

bestander commented Jan 7, 2018 via email

@@ -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> => {
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Choose a reason for hiding this comment

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

can be removed?

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

function not used?

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

workspaces?: Array<string>,
workspaces?: Array<string> | WorkspacesConfig,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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.',
Copy link
Member

Choose a reason for hiding this comment

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

workspaces

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

ok, let's keep it

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

@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 bestander merged commit 4bddb3a into yarnpkg:master Jan 28, 2018
@bestander
Copy link
Member

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

@bestander
Copy link
Member

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

@MrLoh
Copy link

MrLoh commented Jan 28, 2018

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

@bestander
Copy link
Member

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
Copy link
Contributor Author

@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...

agoldis added a commit to agoldis/yarn that referenced this pull request Feb 2, 2018
…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
Copy link

k15a commented Feb 9, 2018

@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
Copy link
Member

arcanis commented Feb 9, 2018

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.

@rebebop
Copy link

rebebop commented Feb 13, 2018

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

k15a commented Feb 13, 2018

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

@rebebop
Copy link

rebebop commented Feb 13, 2018

@k15a nice!

@darthtrevino
Copy link
Contributor

darthtrevino commented Feb 15, 2018

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

@connectdotz
Copy link
Contributor Author

connectdotz commented Feb 16, 2018

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
Copy link

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
Copy link

@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
Copy link
Member

arcanis commented Feb 23, 2018 via email

@k15a
Copy link

k15a commented Feb 23, 2018

@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
Copy link
Member

arcanis commented Feb 23, 2018

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

@natew
Copy link

natew commented Mar 10, 2018

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:

image

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

@connectdotz connectdotz deleted the nohoist branch March 10, 2018 03:34
@connectdotz
Copy link
Contributor Author

connectdotz commented Mar 10, 2018

hi,

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

  1. compare the performance without nohoist. it will help us isolate if the slowness is specifically caused by nohoist.
  2. 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.

risenforward added a commit to risenforward/howdyQ that referenced this pull request Dec 18, 2018
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
We have to wait until yarnpkg/yarn#4979 is merged, as electron-builder can't
resolve the dependencies otherwise.
filippbudko added a commit to filippbudko/graphql-playground that referenced this pull request Jan 16, 2023
We have to wait until yarnpkg/yarn#4979 is merged, as electron-builder can't
resolve the dependencies otherwise.
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