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

Yarn workspaces #1810

Merged
merged 27 commits into from
Sep 8, 2017
Merged

Yarn workspaces #1810

merged 27 commits into from
Sep 8, 2017

Conversation

Hypnosphi
Copy link
Member

@Hypnosphi Hypnosphi commented Sep 6, 2017

Issue: when running yarn in root packages, all the locks for non-root packages are cleared and then recreated with lerna bootstrap. See Hypnosphi#85 (comment)

How to test

  • Upgrade yarn to latest version
    brew upgrade yarn
    
  • In a new terminal session, go to storybook directory and run usual commands (bootstrap, test, etc.)

@Hypnosphi Hypnosphi changed the base branch from master to release/3.3 September 6, 2017 22:06
@Hypnosphi Hypnosphi added ci: do not merge maintenance User-facing maintenance tasks labels Sep 6, 2017
@codecov
Copy link

codecov bot commented Sep 6, 2017

Codecov Report

Merging #1810 into release/3.3 will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1810      +/-   ##
===============================================
- Coverage        23.34%   23.32%   -0.02%     
===============================================
  Files              277      281       +4     
  Lines             6041     6045       +4     
  Branches           703      708       +5     
===============================================
  Hits              1410     1410              
+ Misses            4136     4126      -10     
- Partials           495      509      +14
Impacted Files Coverage Δ
app/react/bin/index.js 0% <ø> (ø)
app/vue/src/server/build.js 0% <ø> (ø) ⬆️
app/vue/bin/index.js 0% <ø> (ø)
app/vue/bin/build.js 0% <ø> (ø)
app/react/bin/build.js 0% <ø> (ø)
app/react/src/server/index.js 0% <ø> (ø) ⬆️
app/react/src/server/build.js 0% <ø> (ø) ⬆️
app/vue/src/server/index.js 0% <ø> (ø) ⬆️
app/react/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/info/src/components/types/PropertyLabel.js 21.05% <0%> (ø) ⬆️
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9c0e84...bd31e8b. Read the comment docs.

@Hypnosphi Hypnosphi changed the title [WIP] Yarn workspaces Yarn workspaces Sep 6, 2017
package.json Outdated
"scripts": {
"bootstrap": "./scripts/bootstrap.js",
"bootstrap:core": "lerna bootstrap --concurrency 1 --npm-client=\"yarn\" --hoist && node ./scripts/hoist-internals.js",
"bootstrap:docs": "cd docs && yarn install",
"bootstrap:core": "yarn install && lerna run prepublish && node ./scripts/hoist-internals.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, at the moment yarn doesn't run lifecycle hooks when bootstrapping workspaces: yarnpkg/yarn#3911
That's why we need lerna run prepublish here

package.json Outdated
"scripts": {
"bootstrap": "./scripts/bootstrap.js",
"bootstrap:core": "lerna bootstrap --concurrency 1 --npm-client=\"yarn\" --hoist && node ./scripts/hoist-internals.js",
"bootstrap:docs": "cd docs && yarn install",
"bootstrap:core": "yarn install && lerna run prepublish && node ./scripts/hoist-internals.js",
Copy link
Member Author

Choose a reason for hiding this comment

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

We can probably now remove yarn install step from yarn bootstrap --reset task, as it runs anyway as part of yarn bootstrap --core

package.json Outdated
@@ -80,7 +88,8 @@
"symlink-dir": "^1.1.0"
},
"engines": {
"node": "node"
"node": "^8.0.0",
Copy link
Member Author

@Hypnosphi Hypnosphi Sep 6, 2017

Choose a reason for hiding this comment

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

This just states the limitation we have de facto

"git add"
],
"*.json": [
"npm run lint:js -- --fix",
"yarn lint:js --fix",
"git add"
],
"*.md": [
Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately when I try to use yarn for *.md tasks, yarn precommit hangs

Copy link
Member

Choose a reason for hiding this comment

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

@@ -15,7 +15,7 @@ log.addLevel('success', 3001, { fg: 'green', bold: true });
log.info(prefix, 'Hoisting internal packages');

const getLernaPackages = () =>
fse.readJson(path.join(__dirname, '..', 'lerna.json')).then(json => json.packages);
fse.readJson(path.join(__dirname, '..', 'package.json')).then(json => json.workspaces);
Copy link
Member Author

Choose a reason for hiding this comment

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

I've noticed that this script always yields Hoisted 0 packages. So either something is broken, or yarn does what's needed. @ndelangen can you please check?

Copy link
Member

Choose a reason for hiding this comment

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

I think yarn workspaces already hoists everything?

Copy link
Member Author

@Hypnosphi Hypnosphi Sep 6, 2017

Choose a reason for hiding this comment

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

Looks like that, I just want to be sure about this

Copy link
Member

Choose a reason for hiding this comment

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

This entire script can be removed if this PR comes thru

"storybook-server": "./dist/server/index.js"
"build-storybook": "./bin/build.js",
"start-storybook": "bin/index.js",
"storybook-server": "bin/index.js"
Copy link
Member Author

@Hypnosphi Hypnosphi Sep 6, 2017

Choose a reason for hiding this comment

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

This change is needed because yarn needs some files to link as binaries, see https://github.com/storybooks/storybook/pull/1810/files#r137413061

"publish": {
"ignore": [
"cra-kitchen-sink",
"crna-kitchen-sink",
Copy link
Member

Choose a reason for hiding this comment

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

@Hypnosphi we don't need these because ... "private: true" in package.json?

Copy link
Member Author

@Hypnosphi Hypnosphi Sep 6, 2017

Choose a reason for hiding this comment

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

]
}
},
"packages": [
Copy link
Member

Choose a reason for hiding this comment

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

@Hypnosphi why don't we need this anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/lerna/lerna#--use-workspaces

If --use-workspaces is true then packages will be overridden by the value from package.json/workspaces.

@shilman
Copy link
Member

shilman commented Sep 6, 2017

@Hypnosphi Awesome!! I'm excited to learn about workspaces. I tried them out on another project and reverted because it didn't seem to do what I wanted, but I'm eager to learn the right way on storybook 😸

command: |
node --version
npm --version
yarn --version
Copy link
Member Author

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

yarn version won't print the correct version anymore, This script isn't providing value really.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe comment it instead? To reenable once docker image gets updated

Copy link
Member

Choose a reason for hiding this comment

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

Meh, it's really not useful the info is in the logs anyway.

@@ -71,6 +77,9 @@ const tasks = {
defaultValue: true,
option: '--core',
command: () => {
log.info(prefix, 'prepublish');
spawn('lerna run prepublish -- --silent');
Copy link
Member Author

Choose a reason for hiding this comment

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

this won't work without global lerna installed

Copy link
Member Author

Choose a reason for hiding this comment

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

and without some package deps like babel as well

Copy link
Member

Choose a reason for hiding this comment

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

isn't lerna a local dependency?

Copy link
Member Author

@Hypnosphi Hypnosphi Sep 7, 2017

Choose a reason for hiding this comment

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

Hmm, yes, didn't know you can run local deps with spawn. The problem is that we effectively yarn install two times in this setup. Why not just once?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, you can't run bootstrap script without root dependencies anyway

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

WORKS ON MY MACHINE

@Hypnosphi Hypnosphi merged commit 8052c61 into release/3.3 Sep 8, 2017
@Hypnosphi Hypnosphi deleted the yarn-workspaces branch September 8, 2017 23:42
- run:
name: "Bootstrapping"
command: |
yarn bootstrap -- --all
yarn bootstrap --core --docs --reactnative --reactnativeapp
Copy link
Member Author

@Hypnosphi Hypnosphi Sep 29, 2017

Choose a reason for hiding this comment

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

What was wrong with --all?
Oh I see, the reset step would remove the cache

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants