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 Gatsby cli support #3740

Closed
wants to merge 6 commits into from
Closed

Conversation

Keraito
Copy link
Contributor

@Keraito Keraito commented Jun 9, 2018

Issue: Fixes #3716

Storybook and Gatsby weren't really compatible. I got storybook working for Gatsby, but then Gatsby's development server wouldn't run anymore.

What I did

After spending some time messing around in the webpack configuration of both projects, I came across gatsbyjs/gatsby#4300. Apparently, Storybook and Gatsby use different versions of Webpack, what does not really go well together and which has been reported multiple times at Gatsby's side gatsbyjs/gatsby#4283 (comment). A detailed description of the underlying issue and work-around is in babel/babel-loader#505 (comment), which is adding specifically version 7.1.1 of babel-loader to the devDependencies.

So I added a generator that adds this particular version of babel-loader, and the detector is based on the gatsby dependency. Gatsby are working on their compatibility with webpack 3, so after that we can remove this work around.


// The specific version (7.1.1) of babel-loader solves a problem caused by Gatsby and Storybook using different versions of Webpack.
// Details on the error can be and the solution was found at: https://github.com/babel/babel-loader/issues/505#issuecomment-324268931.
packageJson.devDependencies['babel-loader'] = '7.1.1';
Copy link
Member

Choose a reason for hiding this comment

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

This can overwrite the version that user had before running getstorybook. Let's at least output a warning in that case

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jun 11, 2018

Codecov Report

Merging #3740 into master will increase coverage by 0.36%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3740      +/-   ##
==========================================
+ Coverage   40.59%   40.95%   +0.36%     
==========================================
  Files         483      482       -1     
  Lines        5747     5696      -51     
  Branches      770      747      -23     
==========================================
  Hits         2333     2333              
+ Misses       3042     3014      -28     
+ Partials      372      349      -23
Impacted Files Coverage Δ
lib/cli/bin/generate.js 0% <0%> (ø) ⬆️

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 7b2b4e0...20da4b5. Read the comment docs.

@Keraito
Copy link
Contributor Author

Keraito commented Jun 11, 2018

Added a warning text after installing storybook. Is it descriptive enough as is?

Also, in the original issue a fix was proposed that is more on the Gatsby side of things. but doesn't require overwriting of dependencies: #3716 (comment). Do we want to do something with that?

@ndelangen
Copy link
Member

What's blocking this from moving forwards?

@Hypnosphi
Copy link
Member

How will it work after #3746 ?

Copy link
Member

@wuweiweiwu wuweiweiwu left a comment

Choose a reason for hiding this comment

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

This is dope!

@Keraito
Copy link
Contributor Author

Keraito commented Jun 22, 2018

How will it work after #3746 ?

I'm not quite sure either, I will have to see after that PR is done. When do you expect that to be done or at a stage that I can test this PR against it, @Hypnosphi?

@Hypnosphi
Copy link
Member

Hypnosphi commented Jun 22, 2018

No estimation, sorry. I'll have a quite poor internet connection in the upcoming week

@Keraito
Copy link
Contributor Author

Keraito commented Jun 22, 2018

No problem, just wanted to see if there was an ETA. I will keep an eye out for the other PR and when that's done test this PR again!

@Hypnosphi
Copy link
Member

Hypnosphi commented Jul 4, 2018

#3746 isn't ready yet, but in contains the relevant changes in lib/cli

Note that it uses babel-loader@8.0.0-beta.4 (see also #3740 (comment))

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

To avoid preliminary merging

@Keraito
Copy link
Contributor Author

Keraito commented Jul 4, 2018

Will try to look into it when I can

@Kikobeats
Copy link

Gatsby community needs this a lot

.then(() => {
logger.log(
chalk.yellow(
'NOTE: Be careful that the specific version 7.1.1 of babel-loader was installed and has overwritten any previously present version, as this is currently required for Storybook and Gatsby to work together.'
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, we should output this only if there was some other version installed

if (!packageJson.dependencies['babel-core'] && !packageJson.devDependencies['babel-core']) {
packageJson.devDependencies['babel-core'] = babelCoreVersion;
}
if (!packageJson.dependencies['babel-runtime'] && !packageJson.devDependencies['babel-runtime']) {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed anymore

'@storybook/addon-actions',
'@storybook/addon-links',
'@storybook/addons',
'babel-core',
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 either install babel-core@bridge + @babel/core here (see installBabel helper), or add a babel config, because our default config is now incompatible with Babel 6

@shilman
Copy link
Member

shilman commented Sep 20, 2018

FYI saw this project on Twitter that might be relevant:

https://github.com/Mathspy/gatsby-storybook-jest-starter

@Keraito
Copy link
Contributor Author

Keraito commented Sep 20, 2018

That looks promising, @shilman. Will look into it when i can this week

@diegolamanno
Copy link

@Keraito Is this for Gatsby V2? I am starting to have problems with Storybook. They all starting to happen after updating.

@Keraito
Copy link
Contributor Author

Keraito commented Oct 4, 2018

I actually made some changes locally but didnt push to remote according to the configuration in https://github.com/Mathspy/gatsby-storybook-jest-starter, which worked but required some specific versions of packages. can't remind of the top of my head but were related to:

    "@babel/core": "^7.0.0-rc.2",
    "@babel/plugin-proposal-class-properties": "^7.0.0-rc.2",
    "@babel/preset-env": "^7.0.0-rc.2",
    "@babel/preset-react": "^7.0.0-rc.2",
    "@babel/runtime": "^7.0.0-rc.2",
    "babel-core": "^7.0.0-bridge.0",
    "babel-runtime": "^6.26.0",

But if @Hypnosphi made it work for Gatsby 2 with the described changed that might be better.

I'm atm kind of busy with some university related stuff, so I hope I can find some time this weekend to look into it further and then report back 🤞

@shilman
Copy link
Member

shilman commented Oct 5, 2018

@Keraito @Hypnosphi I haven't been keeping up on this PR (sorry), but I just wanted to let you know that I've been experimenting with compatibility for SB4:

#4281 (comment)

It seems like the current version of SB4 works fine with Gatsby 2 and that Gatsby 1 is incompatible. @Hypnosphi if you got it working with Gatsby 1, maybe you can comment here or on that issue.

@Hypnosphi
Copy link
Member

if @Hypnosphi made it work for Gatsby 2

Unfortunately not: gatsbyjs/gatsby#8652

@ndelangen
Copy link
Member

@shilman Is this PR still necessary?

We discussed this yesterday how you had a document for this?

Storybook 4 should be 100% compatible with Gatsby 2

@pksunkara
Copy link
Member

I would like to finalize this before moving onto improvements to docs structure and netlify.

@storybook-safe-bot
Copy link
Contributor

Fails
🚫

PR is marked with "do not merge" label.

Generated by 🚫 dangerJS

@shilman
Copy link
Member

shilman commented Oct 15, 2018

@ndelangen The comment in question is here: #4281 (comment)

I propose that we don't worry about Gatsby 1 support and that Gatsby 2 basically works. I might be missing something though!

@Hypnosphi Hypnosphi changed the base branch from master to next November 5, 2018 17:04
@ndelangen ndelangen closed this Dec 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants