Skip to content
This repository has been archived by the owner on Oct 19, 2021. It is now read-only.

chore: Upgrade Storybook to v4 #1498

Merged
merged 8 commits into from
Oct 31, 2018

Conversation

vpicone
Copy link
Contributor

@vpicone vpicone commented Oct 29, 2018

Closes IBM/carbon-components-react#1496

This upgrades storybook to v4. Webpack & babel upgrades, theming, along with a new, backward-compatible parameters API.

Changelog

New

  • @storybook/addon-links (this was registered in addons.js but never added as a dependency)
  • @babel-loader necessary for backward-compatible babel 6 stuff.

Changed

@joshblack
Copy link
Contributor

Thanks for doing this! Will take a look tomorrow to make sure we're good to merge in 🎉

@asudoh
Copy link
Contributor

asudoh commented Oct 30, 2018

Thank you for doing this @vpicone! One question - Is knobs in <Button> working at your end? Just quickly fiddling with this PR and haven't been able to see non-primary buttons, so wanted to make sure I'm not missing anything. Happy to see new stuffs in Storybook BTW!

@vpicone
Copy link
Contributor Author

vpicone commented Oct 30, 2018

@asudoh No, you're right they changed how select works for knobs. I'll dig in to this.

@vpicone
Copy link
Contributor Author

vpicone commented Oct 30, 2018

ugh yarn.lock merge conflicts?

@vpicone
Copy link
Contributor Author

vpicone commented Oct 30, 2018

On the plus side, it looks like the update to addon-info fixes the 'multiple named import' issue that took the descriptions away.

@joshblack
Copy link
Contributor

Hey @vpicone! For yarn merge conflicts, just pull down the changes and re-run yarn. It will resolve them automatically 😄

@vpicone
Copy link
Contributor Author

vpicone commented Oct 30, 2018

Passing stories directly into withInfo() is deprecated in 4.0, so I'm gonna swap it out in favor of the info option and then we should be good to go.

Passing stories directly into withInfo() is deprecated, instead use addDecorator(withInfo) and pass options with the 'info' parameter

@joshblack
Copy link
Contributor

Let us know when you're ready for us to review btw @vpicone!

@vpicone
Copy link
Contributor Author

vpicone commented Oct 30, 2018

@joshblack Should be good to go now. Switched everything off the deprecated withInfo syntax.

@asudoh
Copy link
Contributor

asudoh commented Oct 31, 2018

Thank you for the updates @vpicone! Things seems to work very well except that CARBON_REACT_STORYBOOK_USE_EXTERNAL_CSS=true (ref) seems broken due to Webpack4 move associated with Storybook4 move. Some search tells me that extract-text-webpack-plugin has to be replaced with mini-css-extract-plugin, like below. Could you please try CARBON_REACT_STORYBOOK_USE_EXTERNAL_CSS=true and ensure it works? Thanks!

diff --git a/.storybook/addons.js b/.storybook/addons.js
index 5837958..139bbfe 100644
--- a/.storybook/addons.js
+++ b/.storybook/addons.js
@@ -1,7 +1,7 @@
 import '@storybook/addon-actions/register';
 import '@storybook/addon-knobs/register';
 import '@storybook/addon-links/register';
-import 'storybook-addon-a11y/register';
+import '@storybook/addon-a11y/register';
 import '@storybook/addon-options/register';
 import 'storybook-readme/register';
 import '@storybook/addon-storysource/register';
diff --git a/.storybook/webpack.config.js b/.storybook/webpack.config.js
index 7d67d05..a634c6d 100644
--- a/.storybook/webpack.config.js
+++ b/.storybook/webpack.config.js
@@ -1,5 +1,6 @@
 const path = require('path');
 const ExtractTextPlugin = require('extract-text-webpack-plugin');
+const MiniCssExtractPlugin = require('mini-css-extract-plugin');
 
 const useExperimentalFeatures =
   process.env.CARBON_USE_EXPERIMENTAL_FEATURES === 'true';
@@ -88,17 +89,16 @@ module.exports = {
         test: /\.scss$/,
         use: !useExternalCss
           ? [{ loader: 'style-loader' }, ...styleLoaders]
-          : ExtractTextPlugin.extract({
-              use: styleLoaders,
-            }),
+          : [{ loader: MiniCssExtractPlugin.loader }, ...styleLoaders],
       },
     ],
   },
   plugins: !useExternalCss
     ? []
     : [
-        new ExtractTextPlugin({
-          filename: '[name].[contenthash].css',
+        new MiniCssExtractPlugin({
+          filename: '[name].css',
+          chunkFilename: '[id].css',
         }),
       ],
 };
diff --git a/package.json b/package.json
index 0411d97..42b64e1 100644
--- a/package.json
+++ b/package.json
@@ -155,6 +155,7 @@
     "@babel/plugin-transform-object-assign": "^7.0.0",
     "@babel/preset-env": "^7.1.0",
     "@babel/preset-react": "^7.0.0",
+    "@storybook/addon-a11y": "^4.0.0",
     "@storybook/addon-actions": "^4.0.0",
     "@storybook/addon-info": "^4.0.0",
     "@storybook/addon-knobs": "^4.0.0",
@@ -192,6 +193,7 @@
     "jest-circus": "^23.6.0",
     "lcov2badge": "^0.1.0",
     "lint-staged": "^7.3.0",
+    "mini-css-extract-plugin": "^0.4.4",
     "node-sass": "4.9.2",
     "object-assign": "^4.1.1",
     "postcss-loader": "^3.0.0",
@@ -212,7 +214,6 @@
     "rollup-plugin-uglify": "^3.0.0",
     "sass-loader": "^7.1.0",
     "semantic-release": "^15.0.0",
-    "storybook-addon-a11y": "^3.1.9",
     "storybook-readme": "^4.0.2",
     "string-replace-loader": "^2.1.0",
     "validate-commit-msg": "^2.10.1",

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

LGTM 👍 - Thanks @vpicone!

@vpicone
Copy link
Contributor Author

vpicone commented Oct 31, 2018

There were a few things that required web pack as a peer dependancy. It got installed eventually as a regular dependency of storybook but installing it as a regular dependency gets rid of a few of those errors.

@asudoh
Copy link
Contributor

asudoh commented Oct 31, 2018

@vpicone I saw a similar error - Which seems to have caused by WebPack3 installation still being there by old version of Storybook a11y add-on installation. Upgrading that add-on fixed the issue for me, though your solution is fine for now.

@vpicone
Copy link
Contributor Author

vpicone commented Oct 31, 2018

@asudoh I could be wrong, but I think the loader needs it as a peer dependency at the same level rather than as a dependency of some other peer. Without installing it I get a lot of peer deps warnings.

@asudoh
Copy link
Contributor

asudoh commented Oct 31, 2018

@vpicone Great point, I see at least one dependency having webpack as a peer dependency. Seems that I thought of a different error from what you saw. Thank you for articulating where your earlier comment came from!

@asudoh asudoh merged commit e60b1c8 into carbon-design-system:master Oct 31, 2018
asudoh added a commit that referenced this pull request Nov 1, 2018
asudoh pushed a commit that referenced this pull request Nov 1, 2018
@asudoh
Copy link
Contributor

asudoh commented Nov 1, 2018

Had to revert this PR due to an issue with missing CSS in static build (running yarn build-storybook and running local web server at storybook-static directory). Tried several, including rewinding back to @vpicone's original code, but no luck as of now. Need further investigation - One possibility is that v4 move was too soon. @vpicone Sorry for this and great if you have any ideas - Thanks!

@joshblack
Copy link
Contributor

Oh no! Sorry we didn't catch this during review. Is this related to the change from extract-text-webpack-plugin?

@vpicone
Copy link
Contributor Author

vpicone commented Nov 1, 2018

Hmm weird! everything in the custom web pack config looks correct. Sorry I didn't include the static generation in my testing. I'll definitely be more aware of that next time.

Thinking more about it now, having added webpack 4 as a peer to accommadate the mini-css plugin means we have to make sure the config jives 100% with webpack 4 migration. v3->v4 was pretty minor if I remember but worth investigating I suppose.

@asudoh
Copy link
Contributor

asudoh commented Nov 2, 2018

Ref: Issue# 4690 at https://github.com/storybooks/storybook

@vpicone
Copy link
Contributor Author

vpicone commented Nov 2, 2018

Also, storybookjs/storybook#4645 could be related. I think you were right about jumping the gun @asudoh might have to wait for the dust to settle a bit.

I was having so much fun with theming too haha.

@asudoh
Copy link
Contributor

asudoh commented Nov 2, 2018

You are fast on research @vpicone! I can see your link seems close to what we hit. I enjoyed fiddling with your work, too, BTW!

@carbon-bot
Copy link

🎉 This PR is included in version 6.46.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@vpicone
Copy link
Contributor Author

vpicone commented Nov 5, 2018

@asudoh Hey just wanted to make sure this branch didn't actually get released haha. Carbon-bot might just be overzealous.

@joshblack
Copy link
Contributor

@vpicone I think this commit was included alongside the revert commit so technically it was included, but the effect was not 😅

@asudoh
Copy link
Contributor

asudoh commented Nov 6, 2018

@vpicone @joshblack I think you both got the point - The code was not released. The code won't affect the NPM release either because the stuffs are not in files list.

@vpicone vpicone mentioned this pull request Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants