CHANGE html-webpack-plugin for generate-page-plugin #3919

ndelangen · 2018-07-25T06:37:39Z

Issue: html-webpack-plugin is designed to generate 1 page. generating multiple pages, means creating a new plugin for each page that excludes all chunks but the 1 entrypoint you wanted to generate a page for.

What I did

I've written a webpack(4) plugin that will generate a page for every entrypoint
I've switched our webpack config to make us of it

How to test

Examples should run just fine, including dev-mode and static builds, there should be no changes in functionality.

Performance might be a little bit better since the plugin generatePagePlugin is a lot simpler then the html-webpack-plugin.

This is the first part of sbnext coming to the core 🎉.

igor-dv · 2018-07-25T08:02:21Z

lib/core/src/client/manager/provider.js

@@ -32,7 +32,7 @@ export default class ReactProvider extends Provider {
    }

    const queryString = qs.stringify(queryParams);
-    const url = `iframe.html?${queryString}`;
+    const url = `preview.html?${queryString}`;


Can't we make it backward compatible somehow? I like the name "preview" much more than "iframe", however, there are a lot of tools (like visual testing) that are accessing directly the iframe.html

I think we should actively update storyshots and inform other things that rely on iframe.html if we're making this change.

@igor-dv I'll try it and ask for feedback. Could be that there will be pressure to keep it as is, and I might revert in that case.

We won't know unless we try & ask.

If you have good suggestions on backward compatibility whilst pressuring people to move to the new location (so we don't have to keep supporting both forever) I'd be happy to hear your suggestions?!

@tmeasday ☝️

@oblador 👆

danielduan · 2018-07-25T21:06:53Z

lib/core/src/client/manager/provider.js

@@ -32,7 +32,7 @@ export default class ReactProvider extends Provider {
    }

    const queryString = qs.stringify(queryParams);
-    const url = `iframe.html?${queryString}`;
+    const url = `preview.html?${queryString}`;


I think we should actively update storyshots and inform other things that rely on iframe.html if we're making this change.

ndelangen · 2018-07-25T21:28:30Z

I agree @danielduan please help, what do you mean with update storyshots?

I'm aware some integrations might break, I don't think it will ever have to change after this, but preview makes a ton more sense then iframe IMHO, and it is was the entrypoint was called, and how we all refer to it.

Before releasing this we'll have to inform our known integrators such as Chromatic, Percy, Screener, Applitools.

danielduan · 2018-07-25T21:32:21Z

https://github.com/storybooks/storybook/blob/master/addons/storyshots/storyshots-puppeteer/src/index.js#L44

do a codebase wide search for iframe.html

codecov · 2018-07-25T21:39:20Z

Codecov Report

Merging #3919 into master will decrease coverage by 0.08%.
The diff coverage is 13.04%.

@@            Coverage Diff             @@
##           master    #3919      +/-   ##
==========================================
- Coverage   39.66%   39.58%   -0.09%     
==========================================
  Files         433      433              
  Lines        5445     5467      +22     
  Branches      740      745       +5     
==========================================
+ Hits         2160     2164       +4     
- Misses       2903     2917      +14     
- Partials      382      386       +4

Impacted Files	Coverage Δ
app/react-native/src/server/middleware.js	`0% <0%> (ø)`	⬆️
lib/core/src/server/config/webpack.config.js	`0% <0%> (ø)`	⬆️
lib/core/src/server/config/utils.js	`0% <0%> (ø)`	⬆️
lib/core/server.js	`0% <0%> (ø)`	⬆️
app/react-native/src/server/config.js	`0% <0%> (ø)`	⬆️
...ct-native/src/server/config/webpack.config.prod.js	`0% <0%> (ø)`	⬆️
app/angular/src/server/angular-cli_config.js	`0% <0%> (ø)`	⬆️
...p/react-native/src/server/config/webpack.config.js	`0% <0%> (ø)`	⬆️
lib/core/src/server/config/webpack.config.prod.js	`0% <0%> (ø)`	⬆️
lib/core/src/server/utils.js	`28.57% <54.54%> (+9.52%)`	⬆️

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 ffe59c0...db18161. Read the comment docs.

ndelangen · 2018-07-25T21:40:30Z

Got it, changed it there too.

ndelangen · 2018-07-25T21:41:12Z

I have pinged a few integrators about this potential change, I'm hoping to hear from them how big of an impact it would make.

ndelangen · 2018-07-25T22:31:59Z

ERROR:

Running smoke test in react_native
$ storybook start -p 7007 --smoke-test
=> Loading custom .babelrc from project directory.
=> Loading custom addons config.
=> Using default webpack setup based on "Create React App".
/tmp/storybook/app/react-native/node_modules/html-webpack-plugin/index.js:640
    if (template.indexOf('!') === -1) {
                 ^

TypeError: Cannot read property 'indexOf' of undefined
    at HtmlWebpackPlugin.getFullTemplatePath (/tmp/storybook/app/react-native/node_modules/html-webpack-plugin/index.js:640:18)
    at HtmlWebpackPlugin.apply (/tmp/storybook/app/react-native/node_modules/html-webpack-plugin/index.js:45:34)
    at webpack (/tmp/storybook/node_modules/webpack/lib/webpack.js:37:12)
    at exports.default (/tmp/storybook/app/react-native/dist/server/middleware.js:30:40)
    at new Server (/tmp/storybook/app/react-native/dist/server/index.js:46:50)
    at Object.<anonymous> (/tmp/storybook/app/react-native/dist/bin/storybook-start.js:34:14)
    at Module._compile (internal/modules/cjs/loader.js:654:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:665:10)
    at Module.load (internal/modules/cjs/loader.js:566:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:506:12)
error An unexpected error occurred: "Command failed.
Exit code: 1
Command: sh
Arguments: -c storybook start -p 7007 --smoke-test
Directory: /tmp/storybook/lib/cli/test/run/react_native

Ahh found that the manager html template was 'secretly' shared:
https://github.com/storybooks/storybook/blob/tech/replace-html-webpack-plugin/app/react-native/src/server/config/webpack.config.js#L8-L29

Might be the case for the some other apps too.

I need to move them all over to the new generatePagePlugin
Which means I need to extract it into it's own package...

igor-dv · 2018-07-26T07:49:45Z

If you have good suggestions on backward compatibility

I thought about these few options:

some kind of --compatibility parameter that we can use even for other breaking changes.
still create iframe.html that will just redirect to preview.html
--previewName iframe option 🤷‍♂️

Of course, all these should bump to the user a notification of a feature deprecation.

ndelangen · 2018-07-26T20:38:16Z

I've extracted the generatePagePlugin into it's own repo and package, I think it could be useful outside of storybook.

ndelangen · 2018-07-26T20:40:37Z

Integrators update:
I've heard from Chromatic, Percy and Screener, it's a breaking chance for them, but it's solvable on their end. If we give them enough time.

igor-dv · 2018-07-26T21:24:58Z

app/react-native/src/server/config/webpack.config.js

-import { indexHtmlPath } from '@storybook/core/server';
+import GeneratePagePlugin from 'generate-page-webpack-plugin';
+
+import { getManagerHeadHtml } from '@storybook/core/dist/server/utils';


I think it's better to be exported from core/server

igor-dv · 2018-07-26T21:25:38Z

app/react-native/src/server/config/webpack.config.js

-        version,
+    new GeneratePagePlugin(
+      {
+        template: require.resolve('@storybook/core/dist/server/templates/index.html.ejs'),


And this one 🤷‍♂️

igor-dv · 2018-07-26T21:26:25Z

app/react-native/src/server/config/webpack.config.js

+      {
+        template: require.resolve('@storybook/core/dist/server/templates/index.html.ejs'),
+        // eslint-disable-next-line global-require
+        parser: require('ejs'),


Can't it be also imported up there ☝️

igor-dv · 2018-07-26T21:33:14Z

lib/core/src/server/templates/base-manager-head.html

+<script>
+  if (window.parent !== window) {
+    window.__REACT_DEVTOOLS_GLOBAL_HOOK__ = window.parent.__REACT_DEVTOOLS_GLOBAL_HOOK__;
+    window.__VUE_DEVTOOLS_GLOBAL_HOOK__ = window.parent.__VUE_DEVTOOLS_GLOBAL_HOOK__;


I think we have these both in globals. Anyway it's strange to have them together 🤔

See #102 (comment)

Might look a little strange, but it's convenient I think. 0 complexity

shilman · 2018-07-26T22:33:35Z

I think we should deprecate iframe but keep it around for one major version. It's easy and will give everyone time to adapt

ndelangen · 2018-07-27T06:03:21Z

OK, next major release then

shilman · 2018-07-30T05:02:49Z

@ndelangen To be clear, I mean:

we rename it to preview.html throughout the code
we also have the iframe.html route resolve to preview, and possibly issue a deprecation warning in the dev server
we document the change in release notes and announce that we plan to remove iframe.html
we remove iframe.html in the next major assuming there is no good reason not to

ndelangen · 2018-08-02T07:12:44Z

@shilman yes, we can do that, but I've decided that I should do that in a separate PR, I'm sure you won't mind 👍

I also haven't quite figured our how to effectively do the backwards compatibility technically. I'm guessing the only way it could ever work was if it the static build creates both preview.html AND iframe.html..

I rebased this PR, it should be fairly simple to review now.

ndelangen · 2018-08-02T11:38:24Z

FIXED an angular issue, should be good to go now,

What do you think of the angular fix @alterx @igor-dv ?

igor-dv

Can you please also handle my previous comments? They are mostly cleanups

igor-dv · 2018-08-02T12:56:29Z

lib/core/server.js

-  indexHtmlPath: require.resolve('./src/server/index.html.ejs'),
-  iframeHtmlPath: require.resolve('./src/server/iframe.html.ejs'),
-});
+module.exports = assign({}, defaultWebpackConfig, buildStatic, buildDev, serverUtils, {});


the last {} is redundant

# Conflicts: # lib/core/package.json

# Conflicts: # app/react-native/package.json # lib/core/package.json # yarn.lock

ndelangen · 2018-08-06T10:57:59Z

I will merge this after alpha release today.

diagramatics · 2018-09-06T04:46:41Z

lib/core/src/server/templates/index.html.ejs

+    <title>Storybook</title>
+    <link rel="shortcut icon" href="favicon.ico?v=1" />
+
+    <% if (options.links) { %>


@ndelangen I'm trying to figure out how to plug into this for CSS files we extract out with mini-css-extract-plugin, are there any pointers? Been digging for clues for the past couple of days.

@diagramatics I'm happy to help out, do you have a repo I can use to debug this?

Thanks! I have a repro on cra-kitchen-sink in https://github.com/diagramatics/storybook/tree/repro-mini-css-extract-plugin, with the Webpack config extending the default config and replacing the style-loader with mini-css-extract-plugin.

You can replace mini-css-extract-plugin with extract-text-webpack-plugin to make it in parity with what create-react-app has, but essentially if I understand correctly the issue is that GeneratePagePlugin (and in extension, the Webpack config files to add the plugin in lib/core/src/server/config) doesn't have a way to hook into these common CSS file extraction Webpack plugins to output them into this particular section of the index.html.ejs template (options.links).

https://github.com/storybooks/storybook/blob/013822a39e5ca857efa991a4e67cc3c3c862777b/lib/core/src/server/config/webpack.config.dev.js#L45-L59

Notice there's no options.links specified. Not to mention there is no way we can pass generated CSS files in since this is not in the middle of any hook execution such as additionalChunkAssets to inject the files in.

This is as much as I get digging around the codebase, so let me know if you need more info (or if I'm absolutely wrong 😅). I'm also on Slack by the same username.

XPilot · 2018-09-13T13:13:29Z

This change introduces breaking changes in all projects that use html-webpack-plugins with custom storybook builds. This just murdered my svg-spritesheet generator plugin, thus all svg icons no longer show in storybook output 👎

ndelangen · 2018-09-23T12:32:49Z

@XPilot Let's find a solution to that together, by either extending the generate-page-webpack-plugin or injecting the svgs some other way.

ndelangen added configuration babel / webpack maintenance User-facing maintenance tasks labels Jul 25, 2018

ndelangen self-assigned this Jul 25, 2018

ndelangen requested review from shilman, Hypnosphi and igor-dv July 25, 2018 06:37

ndelangen requested review from alterx and tmeasday as code owners July 25, 2018 06:37

igor-dv reviewed Jul 25, 2018

View reviewed changes

igor-dv added the BREAKING CHANGE label Jul 25, 2018

danielduan approved these changes Jul 25, 2018

View reviewed changes

ndelangen requested a review from thomasbertet as a code owner July 25, 2018 21:39

ndelangen requested review from Gongreg and rmevans9 as code owners July 26, 2018 20:24

igor-dv reviewed Jul 26, 2018

View reviewed changes

ndelangen force-pushed the tech/replace-html-webpack-plugin branch 2 times, most recently from 40fd33d to 07cf773 Compare July 26, 2018 22:04

ndelangen changed the title ~~CHANGE html-webpack-plugin for generate-page-plugin && CHANGE iframe.html to preview.html~~ CHANGE html-webpack-plugin for generate-page-plugin Jul 31, 2018

ndelangen force-pushed the tech/replace-html-webpack-plugin branch from 07dcc76 to 604d9db Compare August 2, 2018 07:15

ndelangen requested a review from usulpro as a code owner August 2, 2018 07:15

ndelangen force-pushed the tech/replace-html-webpack-plugin branch from 6a10a15 to 9be47de Compare August 2, 2018 08:19

ndelangen requested a review from stijnkoopal as a code owner August 2, 2018 08:19

ndelangen removed the BREAKING CHANGE label Aug 2, 2018

CHANGE html-webpack-plugin for generate-page-webpack-plugin

78d8c6c

ndelangen force-pushed the tech/replace-html-webpack-plugin branch 2 times, most recently from 9a7a7aa to 85b73bd Compare August 2, 2018 11:30

igor-dv reviewed Aug 2, 2018

View reviewed changes

CHANGE angular styles & scripts to the iframe-entrypoint

4218659

ndelangen force-pushed the tech/replace-html-webpack-plugin branch from 85b73bd to 4218659 Compare August 2, 2018 20:13

ndelangen mentioned this pull request Aug 5, 2018

Storybook fails to build from InterpolateHtmlPlugin error [alpha.15] #3953

Closed

ndelangen added 2 commits August 5, 2018 18:02

Merge branch 'master' into tech/replace-html-webpack-plugin

7ce9178

# Conflicts: # lib/core/package.json

Merge branch 'master' into tech/replace-html-webpack-plugin

db18161

# Conflicts: # app/react-native/package.json # lib/core/package.json # yarn.lock

ndelangen force-pushed the tech/replace-html-webpack-plugin branch from 353f489 to db18161 Compare August 5, 2018 16:21

ndelangen merged commit 0a33b0b into master Aug 6, 2018

ndelangen deleted the tech/replace-html-webpack-plugin branch August 6, 2018 20:57

diagramatics mentioned this pull request Aug 7, 2018

Webpack 4.16.1 reverses chunk orders on Storybook #3945

Closed

diagramatics reviewed Sep 6, 2018

View reviewed changes

ndelangen mentioned this pull request Oct 12, 2018

CHANGE back to use html-webpack-plugin to keep compatibility with plugins that depend on it #4375

Merged

CHANGE html-webpack-plugin for generate-page-plugin #3919

CHANGE html-webpack-plugin for generate-page-plugin #3919

Conversation

ndelangen commented Jul 25, 2018 • edited Loading

What I did

How to test

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented Jul 25, 2018

danielduan commented Jul 25, 2018

codecov bot commented Jul 25, 2018 • edited Loading

Codecov Report

ndelangen commented Jul 25, 2018

ndelangen commented Jul 25, 2018

ndelangen commented Jul 25, 2018 • edited Loading

igor-dv commented Jul 26, 2018

ndelangen commented Jul 26, 2018

ndelangen commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shilman commented Jul 26, 2018

ndelangen commented Jul 27, 2018

shilman commented Jul 30, 2018

ndelangen commented Aug 2, 2018

ndelangen commented Aug 2, 2018 • edited Loading

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XPilot commented Sep 13, 2018

ndelangen commented Sep 23, 2018

ndelangen commented Jul 25, 2018 •

edited

Loading

codecov bot commented Jul 25, 2018 •

edited

Loading

ndelangen commented Jul 25, 2018 •

edited

Loading

ndelangen commented Aug 2, 2018 •

edited

Loading