Make dependencies more deterministic #1703

Hypnosphi · 2017-08-21T21:44:07Z

stop .gitignoreing yarn.locks
use exact dependencies in package bootstrapped with npm
adjust docs to use yarn in first place

codecov · 2017-08-21T22:15:45Z

Codecov Report

Merging #1703 into release/3.3 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release/3.3    #1703   +/-   ##
============================================
  Coverage        23.12%   23.12%           
============================================
  Files              253      253           
  Lines             5756     5756           
  Branches           677      674    -3     
============================================
  Hits              1331     1331           
- Misses            3943     3946    +3     
+ Partials           482      479    -3

Impacted Files	Coverage Δ
app/vue/src/server/utils.js	`0% <0%> (-32.15%)`	⬇️
lib/ui/src/modules/ui/routes.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/components/layout/usplit.js	`38.7% <0%> (ø)`	⬆️
...ponents/left_panel/stories_tree/tree_decorators.js	`33.33% <0%> (ø)`	⬆️
lib/ui/src/libs/key_events.js	`23.25% <0%> (ø)`	⬆️
lib/components/src/navigation/menu_link.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/PropField.js	`10.86% <0%> (ø)`	⬆️
addons/info/src/components/Props.js	`37.2% <0%> (ø)`	⬆️
addons/info/src/components/PropTable.js	`21% <0%> (ø)`	⬆️
...s/left_panel/stories_tree/tree_decorators_utils.js	`45.23% <0%> (ø)`	⬆️
... and 10 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 6f5b71c...fab716f. Read the comment docs.

danielduan · 2017-08-21T22:55:39Z

CONTRIBUTING.md

@@ -4,6 +4,8 @@ Thanks for your interest in improving Storybook! We are a community-driven proje

 Please review this document to help to streamline the process and save everyone's precious time.

+This guide assumes you're using `yarn` as package manager. You can use `npm` as well, but it would be less deterministic, as we only commit `yarn.lock` to the repo.


We should be impartial and publish a package-lock.json as well imo. There's been problems with npm respecting their own lock files but that's been fixed in [email protected].

This will require running bootstrap two times (using yarn and npm) each time we add or change a dependency

Personally I'm fed up with NPM and would like to just encourage yarn across the board for developing this project. Better platform and much easier than supporting both. Of course end users are welcome to use whatever they want. Thoughts?

I definitely see the concern here and I think yarn is 100% the better package installer. Keeping backward compatibility with npm is the worst. If we're gonna proceed with yarn for the package installer of choice, we should make that very clear and keep to it.

When we phrase it, maybe it should say You should use 'yarn' as your package manager when developing Storybook.

I don't think that we should push this hard. If you use npm after this change your experience will be the same as it is now for everybody =) But if you use yarn, it'll be better

I think the only time someone will actually use the raw npm or yarn command is when doing npm link to link to an external project and npm install --save to add a new dependency. In the new dependency case, we should definitely use yarn since that will actually update the lock file.

I think for simplicity sake, we should just tell contributors to use yarn. I would love to support npm but the unnecessary complexity is really not worth it.

npm install --save

You can't do that with lerna

since that will actually update the lock file.

It will be updated after bootstrapping, which should be done anyway

By the way, workspaces feature just became not experimental: yarnpkg/yarn#4262

If we use it, there will be more sense in stating that we have a yarn-only setup. I'd prefer to introduce it in a separate PR, but I can adjust the wording here if we decide to go that way

We develop storybook using yarn.

If someone wants to install the root with npm, that's of no consequence..
The bootstrap script will use yarn.

We'll assume everyone uses yarn to develop storybook in our documentation.

danielduan · 2017-08-21T22:59:02Z

examples/crna-kitchen-sink/package.json

    "react": "16.0.0-alpha.12",
-    "react-native": "^0.46.1"
+    "react-native": "0.46.1"


I really don't see the benefit of locking our example apps. If a new minor version is released that breaks something, we really have no way of knowing. I know a few of us run these example apps to test if anything breaks before merging or publishing.

I think it's totally valid if we have separate example apps with version numbers locked for different version of react to test though.

Sounds like quite a separate issue. It's good to check that things keep being compatible with latest version, but this check should run on master not on PRs. Because if a new react-native release breaks things, it's master what should be fixed

I understand your reasoning @Hypnosphi.

Generally I'd rather have errors all over the place when we're not compatible with latest.
This will keep us alert and nudges us to resolve it quickly.

It's annoying for PR's but PR branches can always lock it down if the problem comes up.

Having said that, I think in the case of ReactNative we want to be extra cautious. Reactnative itself is on the 0.x.y versioning and is actually using pre-released version of other packages. Locking them down sounds sane.

I'm really interested in what @shilman @tmeasday think about this.

Definitely on the side of keeping up with latest. It will cause some headaches occasionally, but overall I think it will help us much more than it will hurt us.

@ndelangen @shilman isn’t it a task for greenkeeper or similar tool?

# Conflicts: # CONTRIBUTING.md # examples/crna-kitchen-sink/package.json # examples/test-cra/package.json

# Conflicts: # CONTRIBUTING.md

…yarn-lock

danielduan · 2017-08-25T16:18:31Z

CONTRIBUTING.md

+        git clone https://github.com/storybooks/storybook.git
+        cd storybook
+        yarn install
+        yarn run bootstrap -- --core


this should probably be updated to yarn run bootstrap:core

Probably in another PR? Upstream branch has npm run bootstrap -- --core

By the way, it's only by fortune that those are the same things in case of core.
For example, yarn run bootstrap -- --react-native-vanilla is not the same as yarn run bootstrap:react-native-vanilla. Looks like only the bootstrap command should be called immediately. @ndelangen, please correct me if I'm wrong

@danielduan The way it is, is fine. We want people to use the bootstrap script. It's superior to remembering a list of npm script and that way we can change how we order our npm script.

ndelangen · 2017-08-25T22:45:44Z

CONTRIBUTING.md


-cd <your-project>
-npm link @storybook/react
+            ```shcd app/react


This looks broken to me?

Yeah, looks like a merge artifact

ndelangen · 2017-08-25T22:46:11Z

CONTRIBUTING.md

@@ -136,7 +134,7 @@ This project written in ES2016+ syntax so, we need to transpile it before use.
 So run the following command:

 ```sh
-npm run dev
+yarn run dev


Ok, I just wrote verbose versions of commands so that it's easilly mapped to npm. If we go all the way yarn, I'll use shorthands

ndelangen · 2017-08-25T22:46:30Z

CONTRIBUTING.md

 ```

 ### Getting Changes

-After you've done any change, you need to run the `npm run storybook` command every time to see those changes.
+After you've done any change, you need to run the `yarn run storybook` command every time to see those changes.


yarn storybook

Hypnosphi · 2017-08-27T20:04:39Z

I switched all the scripts in CI to use yarn

ndelangen

I have tested

the bootstrapping process
the cra-kitchen-sink
the vue-kitchen-sink
the react-native-vanilla example

ndelangen · 2017-08-27T22:00:10Z

examples/crna-kitchen-sink/package.json

    "react": "16.0.0-alpha.12",
-    "react-native": "^0.46.1"
+    "react-native": "0.46.1"


I understand your reasoning @Hypnosphi.

Generally I'd rather have errors all over the place when we're not compatible with latest.
This will keep us alert and nudges us to resolve it quickly.

It's annoying for PR's but PR branches can always lock it down if the problem comes up.

Having said that, I think in the case of ReactNative we want to be extra cautious. Reactnative itself is on the 0.x.y versioning and is actually using pre-released version of other packages. Locking them down sounds sane.

I'm really interested in what @shilman @tmeasday think about this.

danielduan

Any reason why this is not being merged into master instead of 3.3?

Seems like this is mostly contributor and documentation oriented changes.

tmeasday · 2017-08-28T00:00:29Z

Doesn't lerna end up using npm inside the packages when bootstrap --hoist-ing?

Apart from that I am keen on this change.

Hypnosphi · 2017-08-28T00:49:25Z

@tmeasday why do you think it's so?

tmeasday · 2017-08-28T00:55:06Z

@Hypnosphi I believe they need to run npm --global-style and yarn does not offer that functionality lerna/lerna#852

Luckily npm is smart enough to completely ignore your package.json depending on your package-lock.json so good times are hard by all thanks to this feature and the fact that we aren't checking in package-lock.jsons. /sarcasm

Hypnosphi · 2017-08-28T01:25:28Z

all your package's yarn.lock files will be ignored

Looks like we don't have those

tmeasday · 2017-08-28T01:29:17Z

Looks like we don't have those

Right but npm5 has package-lock.json, which is also currently gitignored

Hypnosphi · 2017-08-28T01:36:11Z

Ok, looks like non-hoistable dependencies rest uncovered by this change. I don't think it's a stopper

tmeasday · 2017-08-28T01:57:26Z

Agreed, although if you can figure out a solution it'd be great ;)

Hypnosphi · 2017-08-28T03:53:49Z

We can try yarn workspaces for bootstrapping =)

shilman · 2017-08-28T04:11:49Z

@Hypnosphi have not had success with yarn workspaces yet (it hoisted so aggressively that it caused problems in my project like lack of .bin directories in my sub-projects) but if you can get something working well i'd definitely love to see it and migrate eventually (if it works well and everybody else is on board)

Hypnosphi · 2017-08-28T04:12:53Z

I’d wait for upcoming 1.0 release first

tmeasday · 2017-08-28T04:24:45Z

Yeah, I think when workspaces does what we need, combined with @ndelangen's hoisting script, I think we'll be good. For now, this is a big improvement, right?

Hypnosphi and others added 2 commits August 22, 2017 00:42

Stop ignoring yarn.locks

a4c5142

Merge branch 'master' into commit-yarn-lock

38e85d3

danielduan reviewed Aug 21, 2017

View reviewed changes

danielduan added the maintenance User-facing maintenance tasks label Aug 21, 2017

Hypnosphi added 2 commits August 22, 2017 23:42

Merge remote-tracking branch 'upstream/master' into commit-yarn-lock

e30e53c

# Conflicts: # CONTRIBUTING.md # examples/crna-kitchen-sink/package.json # examples/test-cra/package.json

merge master

7548bb0

Hypnosphi mentioned this pull request Aug 23, 2017

ADD a CLI for bootstrapping #1216

Merged

1 task

Hypnosphi and others added 5 commits August 23, 2017 22:46

Merge branch 'master' into commit-yarn-lock

c5eec3d

Merge branch 'master' into commit-yarn-lock

cfc06f9

Merge branch 'master' into commit-yarn-lock

0bec0a5

# Conflicts: # CONTRIBUTING.md

Actually commit yarn.locks

1704ab4

Merge remote-tracking branch 'upstream/commit-yarn-lock' into commit-…

e1613d7

…yarn-lock

danielduan reviewed Aug 25, 2017

View reviewed changes

Merge branch 'master' into commit-yarn-lock

1a716a6

ndelangen reviewed Aug 25, 2017

View reviewed changes

Hypnosphi and others added 3 commits August 26, 2017 03:23

Use shorthand commands in docs

ad4ba42

Merge branch 'master' into commit-yarn-lock

fdd699f

Use yarn on CI

41abebf

ndelangen changed the base branch from master to release/3.3 August 27, 2017 20:23

ndelangen added 2 commits August 27, 2017 22:24

Merge branch 'release/3.3' into commit-yarn-lock

7fc2451

Allow lockfiles for packages

9700dbd

ndelangen reviewed Aug 27, 2017

View reviewed changes

new lockfiles

c849239

danielduan mentioned this pull request Aug 27, 2017

Check in package-lock.json #1346

Closed

danielduan approved these changes Aug 27, 2017

View reviewed changes

danielduan added the documentation label Aug 27, 2017

Merge branch 'release/3.3' into commit-yarn-lock

fab716f

Hypnosphi merged commit 039aa83 into release/3.3 Aug 28, 2017

Hypnosphi deleted the commit-yarn-lock branch August 28, 2017 18:41

shilman mentioned this pull request Aug 31, 2017

release/3.3 #1724

Merged

This was referenced Jan 12, 2018

puppeteer versions available: 1.0.0 #2731

Closed

puppeteer versions available: 1.0.0 #2732

Closed

		@@ -4,6 +4,8 @@ Thanks for your interest in improving Storybook! We are a community-driven proje

		Please review this document to help to streamline the process and save everyone's precious time.

		This guide assumes you're using `yarn` as package manager. You can use `npm` as well, but it would be less deterministic, as we only commit `yarn.lock` to the repo.

Make dependencies more deterministic #1703

Make dependencies more deterministic #1703

Conversation

Hypnosphi commented Aug 21, 2017

codecov bot commented Aug 21, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan Aug 23, 2017 • edited Loading

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

danielduan Aug 21, 2017 • edited Loading

Choose a reason for hiding this comment

Hypnosphi Aug 21, 2017 • edited Loading

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

Hypnosphi Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Hypnosphi Aug 25, 2017 • edited Loading

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

Hypnosphi commented Aug 27, 2017

ndelangen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielduan left a comment • edited Loading

Choose a reason for hiding this comment

tmeasday commented Aug 28, 2017

Hypnosphi commented Aug 28, 2017

tmeasday commented Aug 28, 2017

Hypnosphi commented Aug 28, 2017

tmeasday commented Aug 28, 2017

Hypnosphi commented Aug 28, 2017 • edited Loading

tmeasday commented Aug 28, 2017

Hypnosphi commented Aug 28, 2017

shilman commented Aug 28, 2017

Hypnosphi commented Aug 28, 2017

tmeasday commented Aug 28, 2017

codecov bot commented Aug 21, 2017 •

edited

Loading

danielduan Aug 23, 2017 •

edited

Loading

danielduan Aug 21, 2017 •

edited

Loading

Hypnosphi Aug 21, 2017 •

edited

Loading

Hypnosphi Aug 25, 2017 •

edited

Loading

Hypnosphi Aug 25, 2017 •

edited

Loading

danielduan left a comment •

edited

Loading

Hypnosphi commented Aug 28, 2017 •

edited

Loading