-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Support Yarn workspaces to replace bootstrap command #899
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
Conversation
Cool thanks @bestander! |
Thanks @bestander! |
@hzoo @evocateur this is ready for review. |
@bestander Thanks for the great work! |
@Bnaya we are turning it on for jest here jestjs/jest#3906. |
yarn 27.0 is out yeah? can we merge this? I'd like to give it a shot |
Yes, 0.27 is out and marked as stable. |
looking now, thanks for understanding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay!
README.md
Outdated
@@ -840,6 +840,23 @@ May also be configured in `lerna.json`: | |||
} | |||
``` | |||
|
|||
#### --yarnWorkspaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be clearer as --use-workspaces
, with the long-term goal of making it client-independent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
README.md
Outdated
{ | ||
... | ||
"npmClient": "yarn", | ||
"yarnWorkspaces": ["bootstrap"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is an appropriate place to whitelist commands; that's an implementation detail if workspaces are enabled at all. In this PR alone there's already an implicit "ls"
member in this array, and that's not very clear. The useWorkspaces
(the camelization of --use-workspaces
) value should be a boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/Repository.js
Outdated
@@ -55,6 +55,9 @@ export default class Repository { | |||
} | |||
|
|||
get packageConfigs() { | |||
if (this.lernaJson.yarnWorkspaces) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't sufficient because it doesn't pick up any potential --use-workspaces
flag in the CLI args, only the "durable" config from a lerna.json
(mostly equivalent to the relationship of yarn
to a .yarnrc
file, CLI flags always supersede the lerna.json
values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
src/commands/BootstrapCommand.js
Outdated
const { yarnWorkspaces } = this.options; | ||
|
||
if (yarnWorkspaces && (yarnWorkspaces.indexOf("bootstrap") >= 0)) { | ||
this.installRootPackageOnly(callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No symlinking happens in this case? Or is that handled by Yarn now? (I dug through yarnpkg/yarn#3294 but my eyes glazed over before I could figure it out, sorry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, workspaces feature in Yarn creates symlinks in node_modules.
It also treats all workspaces as one installation, so it would hoist symlinks to the root node_modules unless there is a conflict, e.g. jest@next might be using jest-cli@stable in the root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks for the explanation!
Thanks @evocateur, no need to apologize. |
@evocateur, I've switched to --use-workspaces and tested that it works as a CLI config as well. |
I don't think my PR is responsible for appveyor fail |
Oh, npm 5 strikes again.
Export |
That's a great idea! Is that documented anywhere? I'm not against including
that fix in this PR.
…On Wed, Jul 5, 2017 at 9:53 PM, Simen Bekkhus ***@***.***> wrote:
Oh, npm 5 strikes again.
"lerna info version __TEST_VERSION__
+package-3: ┌───────────────────────────────────────────────────────────────┐
+package-3: │ npm update check failed │
+package-3: │ Try running with sudo or get access │
+package-3: │ to the local update config store via │
+package-3: │ sudo chown -R $USER:$(id -gn $USER) C:\\Users\\appveyor\\.config │
+package-3: └───────────────────────────────────────────────────────────────┘
Export NO_UPDATE_NOTIFIER in the tests?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#899 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAV5SyAgrZCxbecRqfYQsfiidSqZLXDks5sLGhigaJpZM4OGFNC>
.
|
https://github.com/yeoman/update-notifier#user-settings I also sent a PR to skip the check on CI automatically. Not sure if it will be accepted, though. sindresorhus/update-notifier#116 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bestander!
{ | ||
... | ||
"npmClient": "yarn", | ||
"use-workspaces": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird that this is not camel case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I can fix momentarily.
Thanks a lot, guys! |
@@ -840,6 +840,22 @@ May also be configured in `lerna.json`: | |||
} | |||
``` | |||
|
|||
#### --use-workspaces | |||
|
|||
Enables integration with [Yarn Workspaces](https://github.com/yarnpkg/rfcs/blob/master/implemented/0000-workspaces-install-phase-1.md) (available since yarn@0.27+). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a a docs page on yarnpkg.com for workspaces?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, I'll work on it in the next 3-4 weeks.
Nice work everyone! |
@bestander sorry to ping you here, but it doesn't seem like wonderful feature tho, i'm really excited about it ! |
@jquense, might be a bug in Yarn. |
I have it up and running in my project, with my .bin files in the appropriate places |
@@ -55,6 +55,9 @@ export default class Repository { | |||
} | |||
|
|||
get packageConfigs() { | |||
if (this.lernaJson.useWorkspaces) { | |||
return this.packageJson.workspaces; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|| [DEFAULT_PACKAGE_GLOB];
? This is not mentioned in the readme, so everything went boom when I tried to use this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yarn does not have a default (PR welcome ;), so it would boom there if this setting is missing.
There is a comment about package.json/workspaces
being used instead of packages
https://github.com/lerna/lerna#--use-workspaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct, I'm just blind! 😀
PR welcome ;)
No doubt
Is it possible to exclude some node modules from beeing hoisted to the root folder? Like the lerna But besides that. Awesome work. Cuts the installation time by half with yarn workspaces for us!! From ~2minutes down to 58secs |
I don't think so regarding no-hoist since that's how yarn workspaces work and lerna is defering to it. |
So i've done some more testing. It appears like yarns install with workspaces doesn't different things from lerna bootstrap. The big things i've notice are:
I've tried a bunch of different configs and setups with the same results. As a replacement for |
Thanks, @jquense. |
Just fyi i created an issue for |
So is |
This thread has been automatically locked because there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description + Motivation and Context
Yarn has a Workspaces feature that implements Lerna's bootstrap logic in a nice "more atomic" and more native from a package manager point of view way.
The progress and all the links to the feature is here yarnpkg/yarn#3294.
Workspaces are still considered "experimental" but we are moving forward and are ready to turn it on for a couple of projects, e.g. jestjs/jest#3906.
This PR integrates Yarn workspaces with Lerna by replacing all the bootstrap/hoisting logic with a simple call to
yarn install
.Also it makes Lerna use package.json/workspaces field as Yarn does.
How Has This Been Tested?
../lerna/bin/lerna.js bootstrap
Types of changes
Checklist: