-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Make dependencies more deterministic #1703
Changes from 10 commits
a4c5142
38e85d3
e30e53c
7548bb0
c5eec3d
cfc06f9
0bec0a5
1704ab4
e1613d7
1a716a6
ad4ba42
fdd699f
41abebf
7fc2451
9700dbd
c849239
fab716f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
## Issues | ||
|
||
No software is bug free. So, if you got an issue, follow these steps: | ||
|
@@ -18,28 +20,26 @@ No software is bug free. So, if you got an issue, follow these steps: | |
|
||
### Testing against `master` | ||
|
||
To test your project against the current latest version of storybook, you can clone the repository and link it with `npm`. Try following these steps: | ||
To test your project against the current latest version of storybook, you can clone the repository and link it with `yarn`. Try following these steps: | ||
|
||
1. Download the latest version of this project, and build it: | ||
|
||
```sh | ||
git clone https://github.com/storybooks/storybook.git | ||
cd storybook | ||
npm install | ||
npm run bootstrap -- --core | ||
``` | ||
```sh | ||
git clone https://github.com/storybooks/storybook.git | ||
cd storybook | ||
yarn install | ||
yarn run bootstrap -- --core | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should probably be updated to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably in another PR? Upstream branch has There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By the way, it's only by fortune that those are the same things in case of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
|
||
2. Link `storybook` and any other required dependencies: | ||
|
||
```sh | ||
cd app/react | ||
npm link | ||
2. Link `storybook` and any other required dependencies: | ||
|
||
cd <your-project> | ||
npm link @storybook/react | ||
```shcd app/react | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks broken to me? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, looks like a merge artifact |
||
yarn link | ||
|
||
# repeat with whichever other parts of the monorepo you are using. | ||
``` | ||
cd <your-project> | ||
yarn link @storybook/react | ||
|
||
# repeat with whichever other parts of the monorepo you are using. | ||
|
||
### Reproductions | ||
|
||
|
@@ -51,13 +51,11 @@ A good way to do that is using the example `cra-kitchen-sink` app embedded in th | |
# Download and build this repository: | ||
git clone https://github.com/storybooks/storybook.git | ||
cd storybook | ||
npm install | ||
npm run bootstrap -- --core | ||
|
||
cd examples/cra-kitchen-sink | ||
yarn install -- --core | ||
yarn run bootstrap | ||
|
||
# make changes to try and reproduce the problem, such as adding components + stories | ||
npm start storybook | ||
yarn start | ||
|
||
# see if you can see the problem, if so, commit it: | ||
git checkout "branch-describing-issue" | ||
|
@@ -71,7 +69,7 @@ git push -u <your-username> master | |
|
||
If you follow that process, you can then link to the github repository in the issue. See <https://github.com/storybooks/storybook/issues/708#issuecomment-290589886> for an example. | ||
|
||
**NOTE**: If your issue involves a webpack config, create-react-app will prevent you from modifying the _app's_ webpack config, however you can still modify storybook's to mirror your app's version of storybook. Alternatively, use `npm run eject` in the CRA app to get a modifiable webpack config. | ||
**NOTE**: If your issue involves a webpack config, create-react-app will prevent you from modifying the _app's_ webpack config, however you can still modify storybook's to mirror your app's version of storybook. Alternatively, use `yarn run eject` in the CRA app to get a modifiable webpack config. | ||
|
||
## Pull Requests (PRs) | ||
|
||
|
@@ -82,7 +80,7 @@ We welcome your contributions. There are many ways you can help us. This is few | |
- Work on [API](https://github.com/storybooks/storybook/labels/enhancement%3A%20api), [Addons](https://github.com/storybooks/storybook/labels/enhancement%3A%20addons), [UI](https://github.com/storybooks/storybook/labels/enhancement%3A%20ui) or [Webpack](https://github.com/storybooks/storybook/labels/enhancement%3A%20webpack) use enhancements and new [features](https://github.com/storybooks/storybook/labels/feature%20request). | ||
- Add more [tests](https://codecov.io/gh/storybooks/storybook/tree/master/packages) (specially for the [UI](https://codecov.io/gh/storybooks/storybook/tree/master/packages/storybook-ui/src)). | ||
|
||
Before you submit a new PR, make you to run `npm test`. Do not submit a PR if tests are failing. If you need any help, create an issue and ask. | ||
Before you submit a new PR, make you to run `yarn test`. Do not submit a PR if tests are failing. If you need any help, create an issue and ask. | ||
|
||
### Reviewing PRs | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
``` | ||
|
||
This will watch files and transpile in watch mode. | ||
|
@@ -146,14 +144,14 @@ This will watch files and transpile in watch mode. | |
First of all link this repo with: | ||
|
||
```sh | ||
npm link | ||
yarn link | ||
``` | ||
|
||
In order to test features you add, you may need to link the local copy of this repo. | ||
For that we need a sample project. Let's create it. | ||
|
||
```sh | ||
npm install --global create-react-app getstorybook | ||
yarn global install --global create-react-app getstorybook | ||
create-react-app my-demo-app | ||
cd my-demo-app | ||
getstorybook | ||
|
@@ -165,12 +163,12 @@ getstorybook | |
Then link storybook inside the sample project with: | ||
|
||
```sh | ||
npm link @storybook/react | ||
yarn link @storybook/react | ||
``` | ||
|
||
### 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
## Release Guide | ||
|
||
|
@@ -195,10 +193,10 @@ git status | |
|
||
# clean out extra files | ||
# WARNING: destructive if you have extra files lying around! | ||
git clean -fdx && yarn | ||
git clean -fdx && yarn install | ||
|
||
# build all the packages | ||
npm run bootstrap -- --all | ||
yarn run bootstrap -- --all | ||
``` | ||
|
||
From here there are different procedures for prerelease (e.g. alpha/beta/rc) and proper release. | ||
|
@@ -209,7 +207,7 @@ From here there are different procedures for prerelease (e.g. alpha/beta/rc) and | |
|
||
```sh | ||
# publish and tag the release | ||
npm run publish -- --concurrency 1 --npm-tag=alpha | ||
yarn run publish -- --concurrency 1 --npm-tag=alpha | ||
|
||
# push the tags | ||
git push --tags | ||
|
@@ -219,14 +217,14 @@ git push --tags | |
|
||
```sh | ||
# publish but don't commit to git | ||
npm run publish -- --concurrency 1 --skip-git | ||
yarn run publish -- --concurrency 1 --skip-git | ||
|
||
# Update `CHANGELOG.md` | ||
# - Edit PR titles/labels on github until output is good | ||
# - Optionally, edit a handwritten description in `CHANGELOG.md` | ||
npm run changelog | ||
yarn run changelog | ||
|
||
# tag the release and push `CHANGELOG.md` and tags | ||
# FIXME: not end-to-end tested! | ||
npm run github-release | ||
yarn run github-release | ||
``` |
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.
We should be impartial and publish a
package-lock.json
as well imo. There's been problems withnpm
respecting their own lock files but that's been fixed in[email protected]
.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 will require running bootstrap two times (using yarn and npm) each time we add or change a dependency
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.
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?
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 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 withyarn
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.
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 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 useyarn
, it'll be betterThere 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 the only time someone will actually use the raw
npm
oryarn
command is when doingnpm link
to link to an external project andnpm install --save
to add a new dependency. In the new dependency case, we should definitely useyarn
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 supportnpm
but the unnecessary complexity is really not worth it.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 can't do that with lerna
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.
It will be updated after bootstrapping, which should be done anyway
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.
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
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.
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.