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

Fixed regression: CSS entries were not picked up for storybook pages (e.g. when using exract-text-webpack-plugin) #1363

Merged

Conversation

joscha
Copy link
Member

@joscha joscha commented Jun 27, 2017

Issue: CSS not displaying when using the exract-text-webpack-plugin, regression introduced in 38b9dfa

What I did

Upgraded from @kadira/storybook@1.x and had the extract-text-webpack-plugin enabled.

Unfortunately my CSS didn't load, encountering the same issue as described in #1104 and most likely #388 and #708.

Using the extract-text-webpack-plugin creates a new entry point however, making the asset list look something like this:

{ manager: 'static/manager.02f732c9002a59214667.bundle.js',
  preview:
   [ 'static/preview.352040d027a7c22331ae.bundle.js',
     'static/preview.1f62768c19a65e75e4259a4c418d292d.css',
     'static/preview.1f62768c19a65e75e4259a4c418d292d.css.map' ] }

and due to https://github.com/storybooks/storybook/blob/34b96aae764a386c35d4db2a057e85236bd8ddfe/app/react/src/server/iframe.html.js#L33 only ever taking the very first asset, the CSS never makes it into iframe.html.

How to test

I added new tests :)

fixes #388
fixes #708
fixes #1104

@codecov
Copy link

codecov bot commented Jun 27, 2017

Codecov Report

Merging #1363 into master will increase coverage by 0.25%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1363      +/-   ##
==========================================
+ Coverage    14.1%   14.35%   +0.25%     
==========================================
  Files         201      201              
  Lines        4609     4611       +2     
  Branches      502      500       -2     
==========================================
+ Hits          650      662      +12     
+ Misses       3521     3520       -1     
+ Partials      438      429       -9
Impacted Files Coverage Δ
app/react/src/server/iframe.html.js 29.26% <83.33%> (+29.26%) ⬆️
app/react/src/server/build.js 0% <0%> (ø) ⬆️
lib/ui/src/libs/key_events.js 23.25% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 41.66% <0%> (ø) ⬆️
app/react/src/demo/Welcome.js 0% <0%> (ø) ⬆️
addons/info/src/components/markdown/htags.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
... and 15 more

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 da6bd65...be7625a. Read the comment docs.

@joscha joscha requested a review from shilman June 27, 2017 01:19
@joscha
Copy link
Member Author

joscha commented Jun 27, 2017

@shilman I assigned you, as you seem to be quite active these days :-) Let me know if anything is unclear! Hoping to get the fix for this regression in asap to de-confuse our extract-text-webpack-plugin-users :)

@joscha
Copy link
Member Author

joscha commented Jun 28, 2017

@usulpro I saw you were last active, would you mind having a look? I am blocked by this, but need a second pair of eyes on it :)

@@ -8,7 +8,7 @@ import url from 'url';
// 'preview.0d2d3d845f78399fd6d5e859daa152a9.css',
// 'static/preview.9adbb5ef965106be1cc3.bundle.js.map',
// 'preview.0d2d3d845f78399fd6d5e859daa152a9.css.map' ]
const urlsFromAssets = assets => {
export const urlsFromAssets = assets => {
Copy link
Member

Choose a reason for hiding this comment

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

who uses this? does it need to be exported?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only the tests, but for those it needs to be exported, yes.

}
assetList.filter(u => re.exec(u)[1] !== 'map').forEach(u => {
Copy link
Member

Choose a reason for hiding this comment

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

u => url?

Copy link
Member Author

@joscha joscha Jun 28, 2017

Choose a reason for hiding this comment

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

done in a9d88cf

@shilman shilman merged commit 768f9ff into storybookjs:master Jun 28, 2017
@joscha joscha deleted the joscha/fix-extract-text-webpack-plugin-usage branch June 28, 2017 11:57
@joscha joscha changed the title fix: allow multiple assets with different types Fixed regression: CSS entries were not picked up for storybook pages (e.g. when using exract-text-webpack-plugin) Jun 28, 2017
@joscha
Copy link
Member Author

joscha commented Jun 28, 2017

Tested in 3.1.7, works as expected:

➜  web git:(joscha/unfork-storybook) yarn build-storybook
yarn build-storybook v0.24.6
@storybook/react v3.1.7

=> Loading custom addons config.
=> Loading custom webpack config (extending mode).
Building storybook ...
ts-loader: Using typescript@2.3.4 and /Users/joscha/Development/canva/web/tsconfig.ts-loader.json
✨  Done in 382.09s.
➜  web git:(joscha/unfork-storybook) ✗ cat target/storybook/iframe.html | grep link
        <link rel='stylesheet' type='text/css' href='preview.89d82dfdc51e6aeaba00b6d8a4785cf3.css'>

@danielmcgrath
Copy link

@joscha do you happen to have a sample project you could point to? I'm on 3.1.7 and can't for the life of me get webpack to actually generate the CSS file. We haven't hit any issues with extract-text-plugin in other projects so I'm wondering if there's a storybook-specific step I'm missing here.

@joscha
Copy link
Member Author

joscha commented Jun 29, 2017

@danielmcgrath we use extension mode (e.g. we have an additional webpack.config.js in the conf/ folder and then we do:

  const extractCSS = new ExtractTextPlugin({
    // The filename does not contain a hash in dev and local such that conf/head.html used by
    // Storybook can refer directly to the static filename in Storybook.
    filename: options.hashOutputFileNames ? '[name].[contenthash].css' : '[name].css',
    allChunks: true,
    ignoreOrder: true,
  });

// ...
plugins: [
  extractCSS
]

that's all that is needed for a preview.c0ffee.css to be generated and picked up in the HTML.

@WellerQu
Copy link

Hi, @joscha , We very love storybook. But we got the error:

ERROR in ./src/core/view/style.sass
Module build failed: Error: "extract-text-webpack-plugin" loader is used without the corresponding plugin, refer to https://github.com/webpack/extract-text-webpack-plugin for the usage example
    at Object.module.exports.pitch (/Users/qiuwei/Workspace/rongcaptial-ui/node_modules/extract-text-webpack-plugin/loader.js:25:9)
 @ ./src/core/view/index.js 33:13-36
 @ ./src/core/index.js
 @ ./stories/view.story.js
 @ ./build/config.js
 @ multi ./~/@storybook/react/dist/server/config/polyfills.js ./~/@storybook/react/dist/server/config/globals.js ./~/webpack-hot-middleware/client.js?reload=true ./build/config.js

My custorm webconfig is:

rules: [
            {
                // use sass-loader for *.sass files
                test: /\.sass/i,
                use: ExtractTextPlugin.extract({ 
                    fallback: 'style-loader',
                    use: [
                        { 
                            loader: `css-loader?modules&localIdentName=${ __DEV__ ? '[path][name]__[local]--[hash:base64:5]' : '[hash:base64]' }`,
                        },
                        {
                            loader: 'postcss-loader',
                            options: {
                                plugins: [
                                    precss,
                                    autoprefixer
                                ]
                            }
                        },
                        {
                            loader: "sass-loader",
                        },
                    ],
                }),
                exclude: /node_modules/
            },
]
"@storybook/react": "^3.1.7", from [here](https://storybook.js.org/basics/slow-start-guide/)
"webpack": "2.2.0",
"webpack-merge": "^4.1.0",

Any ideas, please

@joscha
Copy link
Member Author

joscha commented Jul 1, 2017 via email

@WellerQu
Copy link

WellerQu commented Jul 3, 2017

@joscha Yes, you are right. I forgot it. It works now. Thanks.

@janusch
Copy link

janusch commented Sep 28, 2017

@joscha I somehow cannot get extract styles to work with extending the config based on your example above. Could you expand on your example, or show a whole webpack config.? How do you get the env check into the webpack config when in "extending mode"?
when i run build-storybook there are no stylesheets in the output dir, nor is it added to index.html or iframe.html.
Can you see any oddities or tell me what I am missing?

const path = require('path');
const ExtractTextPlugin = require('extract-text-webpack-plugin');

const extractCSS = new ExtractTextPlugin({
  filename: '../build/static/[name].css',
  allChunks: true,
  ignoreOrder: true,
});

module.exports = {
  module: {
    rules: [
      {
        test: /\.md$/,
        use: 'raw-loader',
      },
      {
        test: /\.(jpg|png)$/,
        use: "file-loader"
      },
      {
        test: /\.css$/,
        include: path.resolve(__dirname, '../src/'),
        exclude: path.resolve(__dirname, '../node_modules/'),
        use: extractCSS.extract({
          fallback: 'style-loader',
          use: [
            {
              loader: 'css-loader',
              options: {
                importLoaders: 1,
                modules: true
              },
            },
            {
              loader: 'postcss-loader',
              options: {
                plugins: function(loader) {
                  return [require('autoprefixer')];
                },
              },
            },
          ],
        }),
      },
    ],
  },
  plugins: [
    extractCSS
  ],
};

@ndelangen
Copy link
Member

@janusch Would you be able to try the 3.3 alpha release?

We have changed the way entry points are added there, and the issue you're having should be resolved. could you check it out?

@janusch
Copy link

janusch commented Sep 29, 2017

@ndelangen Hey, thank you for getting back to me that quickly!
I have just run build-storybook with extract-text-webpack-plugin and it does now successfully create, copy and include the preview.css file.

However, there is another error related to webpack now. Maybe that is due to some other breaking change? It was the first issue in storybook that refers to the same error: storybook-eol/storybook-addons#1 TypeError: Cannot read property 'emit' of null at emitAddReadme
I am using the storybook-readme addon.

One more little thing is that as soon as I am wrapping the webpack config object in a function returning the object, to get the env var, storybook is changing from extended mode to full-control mode. Is there another way to work with an extended config and still make use of env for production development, etc. differentiation?

Thank you for working so hard on storybook, it is really becoming a complex product, with a lot of great features!

@janusch
Copy link

janusch commented Sep 29, 2017

One more little thing is that as soon as I am wrapping the webpack config object in a function returning the object, to get the env var, storybook is changing from extended mode to full-control mode. Is there another way to work with an extended config and still make use of env for production development, etc. differentiation?

this one I got resolved!

I am still having the 'emit' error described above. I am running the latest version of the package. storybook-readme@3.0.6

Please let me know, if I should test something else, looking forward to get the latest alpha into a stable build asap.

@janusch
Copy link

janusch commented Sep 29, 2017

I got the remaining issues fixed. Just had to upgrade everything else to 3.3.0-alpha.0 as well.

Thank you for the help, storybook rocks!

@bsr203
Copy link

bsr203 commented Oct 3, 2017

sorry I am bit lost as first time using storybook. Do I need a custom webpack conf to load css. I have an app based on CRA, and in stories/index.js, I am importing the main css file as import '../index.css';. I don't see any style applied.

Uses "@storybook/react": "^3.2.12".

Do I need to use 3.3.0-alpha.0?

thanks for any help.

EDIT:
sorry for the noise. It is working well, I didn't include one of the css file which was used by the component. No conf change needed. Thanks.

@shahankit
Copy link

@shilman Same issue as @bsr203. I'm using full control mode and I could not find any plugin in default config passed so I added new config using config.plugins.push(new ExtractTextPlugin('component-styles.[contenthash].css')); and also update /\.css$/ to ExtractTextPlugin.extract.

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.

9 participants