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

Add React Styleguidist #2044

Merged
merged 1 commit into from
Jun 27, 2017
Merged

Add React Styleguidist #2044

merged 1 commit into from
Jun 27, 2017

Conversation

sapegin
Copy link
Contributor

@sapegin sapegin commented Apr 28, 2017

This is a second try to add React Styleguidist to CRA docs after #921, where we’ve decided that CRA integration in Styleguidist wasn’t good enough. Since then we’ve greatly improved first time experience and out-of-the-box CRA support.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

Sorry, been a bit overwhelmed. Is this ready? Can you rebase please?

@sapegin
Copy link
Contributor Author

sapegin commented Jun 27, 2017

@gaearon Done! Looking forward to your review 🍕

@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

Did this section accidentally disappear from TOC? Can you add it back?

@sapegin
Copy link
Contributor Author

sapegin commented Jun 27, 2017

Looks like it wasn’t there in the first place ;-/ I’ve updated the TOC.

@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

There's also one in main README. Is it there?

@sapegin
Copy link
Contributor Author

sapegin commented Jun 27, 2017

Nope, it wasn’t there too ;-)

@gaearon gaearon added this to the 1.0.8 milestone Jun 27, 2017
@gaearon gaearon merged commit 12a2e42 into facebook:master Jun 27, 2017
@gaearon
Copy link
Contributor

gaearon commented Jun 27, 2017

Looks good. Thank you!

@sapegin sapegin deleted the patch-1 branch June 27, 2017 21:00
@sapegin
Copy link
Contributor Author

sapegin commented Jun 27, 2017

Awesome, thanks! 🍕

First install Styleguidist and peer dependencies from npm:

```sh
npm install --save-dev react-styleguidist webpack
Copy link
Contributor

Choose a reason for hiding this comment

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

I just noticed this. This is a bit scary.

People had a lot of issues trying to install webpack directly into CRA projects. Things would randomly break, and they would be running a wrong or incompatible version.

Why is it required for Styleguidist?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's possible to not make it a peer, and just rely on it existing, this would be awesome. It's always going to exist in CRA projects, but making it a peer puts user in charge of managing webpack versions which goes directly against goals of this project, and will break people's setups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that users should be able to choose their own version of webpack (if it’s supported by Styleguidist of course). What if we add it as a dependency with a range like >=1 <4? Would npm properly dedupe them and use the same webpack version for Styleguidist and the host project or CRA?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully yes but I'd prefer to avoid relying on this. It doesn't always work.

In my practice peers are often just not worth it either. You can put require in a try catch and show a graceful crash message if you'd like. IMO this is a more solid way to accomplish what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, we already has a webpack version check so we could just use it to show a warning if it’s not installed or version is not supported. I guess majority of our users already have webpack anyway ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind making this in a patch today? so I can remove this instruction before cutting a release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’m going to make a release as soon as I test this fix ;-)

gaearon added a commit that referenced this pull request Jun 27, 2017
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

A few other things (I'll fix myself but wanted to note).


Then, add these scripts to your `package.json`:

```sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a shell file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True ;-|


```sh
{
"scripts": {
Copy link
Contributor

Choose a reason for hiding this comment

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

User naïvely following this instruction will replace the whole scripts section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you recommend here?

sapegin added a commit to styleguidist/react-styleguidist that referenced this pull request Jun 28, 2017
Check webpack availability and version at run time instead of using peerDependencies to allow usage of build tools that contains webpack as their own dependency, like Create React App.

See discussion here: facebook/create-react-app#2044
@gaearon
Copy link
Contributor

gaearon commented Jun 28, 2017

I did a followup in #2660.

@sapegin
Copy link
Contributor Author

sapegin commented Jun 28, 2017

Awesome! I’ve just made a new release without webpack peer dependency.

@gaearon gaearon mentioned this pull request Jun 28, 2017
romaindso pushed a commit to romaindso/create-react-app that referenced this pull request Jul 10, 2017
wmonk referenced this pull request in wmonk/create-react-app-typescript Aug 7, 2017
morgs32 pushed a commit to BrickworkSoftware/create-react-app that referenced this pull request Sep 1, 2017
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants