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

Make dependencies more deterministic #1703

Merged
merged 17 commits into from
Aug 28, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ coverage/
*.lerna_backup
build
packages/examples/automated-*
yarn.lock
/**/LICENSE
docs/public
packs/*.tgz
Expand Down
64 changes: 31 additions & 33 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

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 with npm respecting their own lock files but that's been fixed in npm@5.3.

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 will require running bootstrap two times (using yarn and npm) each time we add or change a dependency

Copy link
Member

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?

Copy link
Member

@danielduan danielduan Aug 23, 2017

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

npm install --save

You can't do that with lerna

Copy link
Member Author

Choose a reason for hiding this comment

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

since that will actually update the lock file.

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

Copy link
Member Author

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

Copy link
Member

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.


## Issues

No software is bug free. So, if you got an issue, follow these steps:
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be updated to yarn run bootstrap:core

Copy link
Member Author

@Hypnosphi Hypnosphi Aug 25, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Hypnosphi Hypnosphi Aug 25, 2017

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

This looks broken to me?

Copy link
Member Author

Choose a reason for hiding this comment

The 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

Expand All @@ -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"
Expand All @@ -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)

Expand All @@ -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

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

Choose a reason for hiding this comment

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

yarn dev

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

Choose a reason for hiding this comment

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

yarn storybook


## Release Guide

Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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
```
18 changes: 9 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,30 +92,30 @@ We welcome contributions to Storybook!

### Development scripts

#### `npm run bootstrap`
#### `yarn run bootstrap`

> Installs package dependencies and links packages together - using lerna

#### `npm run publish`
#### `yarn run publish`

> Push a release to git and npm
> will ask for version in interactive mode - using lerna.

#### `npm run lint`
#### `yarn run lint`

> boolean check if code conforms to linting rules - uses remark & eslint

- `npm run lint:js` - will check js
- `npm run lint:md` - will check markdown + code samples
- `yarn run lint:js` - will check js
- `yarn run lint:md` - will check markdown + code samples

- `npm run lint:js -- --fix` - will automatically fix js
- `npm run lint:md -- -o` - will automatically fix markdown
- `yarn run lint:js -- --fix` - will automatically fix js
- `yarn run lint:md -- -o` - will automatically fix markdown

#### `npm run test`
#### `yarn run test`

> boolean check if unit tests all pass - uses jest

- `npm run test:watch` - will run tests in watch-mode
- `yarn run test:watch` - will run tests in watch-mode

### Backers

Expand Down
6 changes: 3 additions & 3 deletions docs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ This is the source for [storybook.js.org](https://storybook.js.org). It document
### Usage

```sh
npm i
npm run develop
npm run storybook
yarn install
yarn run develop
yarn run storybook
```

### Edit Documentation
Expand Down
Loading