Use HtmlWebpackPlugin to import all assets (importing chunks in order) #1775

thomasbertet · 2017-08-31T07:12:17Z

Issue: #1411

What I did

Removing manuel insertion of assets inside a index/iframe.html template.
Use HtmlWebpackPlugin to import in correct order all chunks.

How to test

Have a build with multiple split points, everything should be loaded in order.

Is this testable with jest or storyshots?
Not sure how yet.. will do.

Does this need a new example in the kitchen sink apps?
No.

Does this need an update to the documentation?
No.

If your answer is yes to any of these, please make sure to include it in your PR.

codecov · 2017-08-31T07:18:35Z

Codecov Report

Merging #1775 into release/3.3 will decrease coverage by 0.08%.
The diff coverage is 20%.

@@               Coverage Diff               @@
##           release/3.3    #1775      +/-   ##
===============================================
- Coverage        23.14%   23.05%   -0.09%     
===============================================
  Files              253      248       -5     
  Lines             5756     5703      -53     
  Branches           691      684       -7     
===============================================
- Hits              1332     1315      -17     
+ Misses            3924     3889      -35     
+ Partials           500      499       -1

Impacted Files	Coverage Δ
app/react/src/server/config/webpack.config.prod.js	`0% <ø> (ø)`	⬆️
app/react/src/server/config/webpack.config.js	`0% <ø> (ø)`	⬆️
app/vue/src/server/config/webpack.config.js	`0% <ø> (ø)`	⬆️
app/vue/src/server/config/webpack.config.prod.js	`0% <ø> (ø)`	⬆️
lib/ui/src/modules/ui/components/layout/index.js	`76.27% <ø> (ø)`	⬆️
...i/src/modules/ui/components/stories_panel/index.js	`100% <ø> (ø)`	⬆️
app/react/src/server/middleware.js	`0% <0%> (ø)`	⬆️
app/vue/src/server/middleware.js	`0% <0%> (ø)`	⬆️
app/react/src/server/build.js	`0% <0%> (ø)`	⬆️
...ct-native/src/server/config/webpack.config.prod.js	`0% <0%> (ø)`	⬆️
... and 48 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 db01fac...99e9ab3. Read the comment docs.

ndelangen

Code looks good to me,

Should we add this behavior to the cra-kitchen-sink to make sure it actually works?

The unit tests for this tests only a really small part of the puzzle. A huge portion is inside webpack, and we'd like to verify it works, not just know the order is correct.

cc @tmeasday @Hypnosphi @shilman

Hypnosphi · 2017-08-31T11:05:37Z

I agree.
@thomasbertet please add a custom webpack config for examples/cra-kitchen-sinks which will, for example, create a separate vendor chunk

thomasbertet · 2017-08-31T12:40:48Z

Fine by me, will add =) I'll let you know.

thomasbertet · 2017-08-31T16:22:26Z

@Hypnosphi after some digging around CRA, we cannot split bundles yet facebook/create-react-app#2206 (at least, not using the webpack config). Meaning, splitting vendors & app cannot be done easily using the CommonChunksPlugin. Correct me if I'm wrong!

I may need then to set some split points inside the code of cra-kitchen-sinks, WDYT ? This way I will be able to give it a name (using the magic comment of webpack import syntax) and test some cases.

Hypnosphi · 2017-08-31T16:31:28Z

I mean a custom webpack config for storybook, not for app itself

thomasbertet · 2017-09-01T08:15:29Z

Ok then, here is my findings:

When you have split points in your app, you need to load them after preview.bundle.js (preview contains webpack's bootstrapping)
When you use CommonsChunkPlugin in .storybook/webpack.config.js to extract, say, a "vendor" bundle, then you will want to load it before preview.bundle.js. (vendor contains webpack's bootstrapping)

These two use cases could not be solved using the quick fix in this PR in its current state.
We might want to add extra sorting to assets so that they are inserted in the correct order depending on the dependency graph.

When using the statsJson, the https://webpack.js.org/api/stats/#chunk-objects has a parent prop we can use to reorder correctly the chunks.

Maybe we should consider adding a similar logic as HtmlWebpackPlugin there: https://github.com/jantimon/html-webpack-plugin/blob/master/lib/chunksorter.js ?

Please correct me if I'm misunderstanding something, any help appreciated 👍

Also, maybe @SpaceK33z you might want to give us a thought as its webpack's related :)

Thanks!

SpaceK33z · 2017-09-01T09:34:50Z

Hi, this part of webpack is not my area of expertise, but what I can say is that in webpack 4 the order should not matter anymore (source, ctrl+f on 'pure jsonp'). But I realize this will take some time so for now this info is useless 😆.

Hypnosphi · 2017-09-01T10:40:28Z

Maybe we should consider adding a similar logic as HtmlWebpackPlugin

Maybe we should consider just using the plugin =)

thomasbertet · 2017-09-01T16:31:16Z

@Hypnosphi I implemented a solution to this with HtmlWebpackPlugin.

I tried my best with the little knowledge I have from the codebase to not break anything.

Could you find the time to try it, and guide me through where I can create test cases to improve this PR ?

Thanks in advance !

ps: A a side note, I had issues with addons channels, couldn't really fix it, so I commented out everything addon related to test, with hope this is a development related issue. Please let me know if some is not working :)

ndelangen · 2017-09-01T22:58:01Z

I like where this is going! Great work @thomasbertet

Hypnosphi · 2017-09-02T00:14:09Z

Works great! Please make the same chages to app/vue (yeah, the duplication isn't cool, but it's kinda by design as far as I know)

thomasbertet · 2017-09-02T10:00:51Z

Thanks @ndelangen ! Really appreciate this :-)
It feels good to finally help improve storybook.

Will definately do the same for vue. Thanks @Hypnosphi !

thomasbertet · 2017-09-04T07:07:38Z

@Hypnosphi I added it for vue as well.

Ready to take it to the next level, please let me know if I need to do something else.

ndelangen · 2017-09-04T21:03:10Z

I will test this now.

ndelangen

Tested the regular and static build for cra and vue, works great!

I added a custom webpack config with a vendor chunk for vue too.

Hypnosphi · 2017-09-04T23:55:38Z

@ndelangen should we probably merge it into release/3.3 not master?

Hypnosphi

LGTM

shilman · 2017-09-05T00:00:53Z

I noticed changes for react/vue but not for RN, which also has a very similar webpack/preview setup (in addition to the device side of things). Do we need to change RN as well?

Hypnosphi · 2017-09-05T00:02:26Z

@shilman As far as I understand, RN app doesn't have a preview entry in webpack config:
https://github.com/storybooks/storybook/blob/master/app/react-native/src/server/config/webpack.config.js#L9

It probably could still benefit from HtmlWebpackPlugin, but #1411 looks irrelevant to it

ndelangen · 2017-09-05T05:17:25Z

@thomasbertet I think we should try to keep the apps in sync as much as possible.

Do you want to bring these changes to the RN app as well? Or do you want me to do it?

thomasbertet · 2017-09-05T06:00:32Z

Sure I can try. Just to be certain, we would like all of them to use the same files, right ? As @Hypnosphi mentioned, is it a design we would like to keep ? Thanks !

…

On Tue, Sep 5, 2017, 07:17 Norbert de Langen ***@***.***> wrote: @thomasbertet <https://github.com/thomasbertet> I think we should try to keep the apps in sync as much as possible. Do you want to bring these changes to the RN app as well? Or do you want me to do it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1775 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACbvFr1WUYY8U6eoXxLERpDG38nCwl_6ks5sfNlqgaJpZM4PIaYe> .

ndelangen · 2017-09-05T11:06:30Z

After Angular app has been merged in, we will be refactoring out most of the duplication.

So no it's not our goal to keep it exactly like this; though it's also not a goal to abstract away all forms of duplication. Nothing hurts a codebase more than early abstraction.

# Conflicts: # app/react/src/server/index.html.js # app/vue/src/server/index.html.js

thomasbertet · 2017-09-06T09:14:16Z

@ndelangen this is it =) I added the same behavior for RN & the CRNA example.

ndelangen · 2017-09-06T12:10:21Z

I will test it @thomasbertet, you're awesome! 💪

ndelangen · 2017-09-06T12:57:55Z

Tested both the crna-example and the react-native-vanilla ✅

thomasbertet · 2017-09-06T13:08:26Z

Great ! Thanks @ndelangen 👍

ndelangen · 2017-09-06T13:14:49Z

Congrats @thomasbertet !

I hope more PRs like this will follow! 🙇

thomasbertet mentioned this pull request Aug 31, 2017

Codesplit chunks are out of order causes "webpackJsonp is not defined" error #1411

Closed

ndelangen reviewed Aug 31, 2017

View reviewed changes

ndelangen assigned thomasbertet and ndelangen Aug 31, 2017

ndelangen added bug configuration babel / webpack labels Aug 31, 2017

thomasbertet changed the title ~~Load chunks after the preview bundle~~ Use HtmlWebpackPlugin to import all assets (importing chunks in order) Sep 1, 2017

Hypnosphi self-requested a review September 1, 2017 23:50

Thomas Bertet added 4 commits September 4, 2017 09:06

load chunks after the preview bundle

df1fa10

use HtmlWebpackPlugin & custom webpack.config.js in cra-kitchen-sink

f422481

remove useless test file since HtmlWebpackPlugin is taking over

174db1a

use HtmlWebpackPlugin for vue

e3020ce

thomasbertet force-pushed the fix-ordering-chunk-load branch from 9aaecd9 to e3020ce Compare September 4, 2017 07:06

ndelangen added 3 commits September 4, 2017 23:03

Merge branch 'master' into fix-ordering-chunk-load

a9c0c06

ADD custom webpack config for vue kitchen sink as with cra-kitchen-sink

5438db4

Fix for lint-staged see: lint-staged/lint-staged#225 (comment)

2b0644c

ndelangen approved these changes Sep 4, 2017

View reviewed changes

ndelangen mentioned this pull request Sep 4, 2017

Replace UglifyJS with babel-minify #1733

Closed

Hypnosphi approved these changes Sep 4, 2017

View reviewed changes

Hypnosphi changed the base branch from master to release/3.3 September 5, 2017 00:22

ndelangen added 3 commits September 6, 2017 08:18

Merge branch 'release/3.3' into thomasbertet/fix-ordering-chunk-load

8dd71b2

# Conflicts: # app/react/src/server/index.html.js # app/vue/src/server/index.html.js

FIX snapshots

faa4803

use HtmlWebpackPlugin for RN & CRNA

bd6d8a6

thomasbertet force-pushed the fix-ordering-chunk-load branch from 1e6d0e0 to bd6d8a6 Compare September 6, 2017 09:12

Merge branch 'release/3.3' into fix-ordering-chunk-load

99e9ab3

ndelangen requested review from tmeasday and removed request for tmeasday September 6, 2017 12:59

ndelangen merged commit 40c652d into storybookjs:release/3.3 Sep 6, 2017

thomasbertet deleted the fix-ordering-chunk-load branch September 6, 2017 13:08

samouss mentioned this pull request Dec 28, 2017

3.3.0-alpha.4 preview-head does not work with a custom configDir #2501

Closed

Hypnosphi mentioned this pull request Jan 9, 2018

3.3.4 -> 3.3.5 ENOENT: no such file or directory, stat '.../node_modules/@storybook/react/dist/server/public/index.html #2680

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use HtmlWebpackPlugin to import all assets (importing chunks in order) #1775

Use HtmlWebpackPlugin to import all assets (importing chunks in order) #1775

thomasbertet commented Aug 31, 2017 •

edited

Loading

codecov bot commented Aug 31, 2017 •

edited

Loading

ndelangen left a comment

Hypnosphi commented Aug 31, 2017

thomasbertet commented Aug 31, 2017

thomasbertet commented Aug 31, 2017 •

edited

Loading

Hypnosphi commented Aug 31, 2017 •

edited

Loading

thomasbertet commented Sep 1, 2017

SpaceK33z commented Sep 1, 2017 •

edited

Loading

Hypnosphi commented Sep 1, 2017

thomasbertet commented Sep 1, 2017 •

edited

Loading

ndelangen commented Sep 1, 2017

Hypnosphi commented Sep 2, 2017

thomasbertet commented Sep 2, 2017

thomasbertet commented Sep 4, 2017

ndelangen commented Sep 4, 2017

ndelangen left a comment

Hypnosphi commented Sep 4, 2017

Hypnosphi left a comment

shilman commented Sep 5, 2017

Hypnosphi commented Sep 5, 2017 •

edited

Loading

ndelangen commented Sep 5, 2017

thomasbertet commented Sep 5, 2017 via email

ndelangen commented Sep 5, 2017

thomasbertet commented Sep 6, 2017

ndelangen commented Sep 6, 2017

ndelangen commented Sep 6, 2017

thomasbertet commented Sep 6, 2017

ndelangen commented Sep 6, 2017

Use HtmlWebpackPlugin to import all assets (importing chunks in order) #1775

Use HtmlWebpackPlugin to import all assets (importing chunks in order) #1775

Conversation

thomasbertet commented Aug 31, 2017 • edited Loading

What I did

How to test

codecov bot commented Aug 31, 2017 • edited Loading

Codecov Report

ndelangen left a comment

Choose a reason for hiding this comment

Hypnosphi commented Aug 31, 2017

thomasbertet commented Aug 31, 2017

thomasbertet commented Aug 31, 2017 • edited Loading

Hypnosphi commented Aug 31, 2017 • edited Loading

thomasbertet commented Sep 1, 2017

SpaceK33z commented Sep 1, 2017 • edited Loading

Hypnosphi commented Sep 1, 2017

thomasbertet commented Sep 1, 2017 • edited Loading

ndelangen commented Sep 1, 2017

Hypnosphi commented Sep 2, 2017

thomasbertet commented Sep 2, 2017

thomasbertet commented Sep 4, 2017

ndelangen commented Sep 4, 2017

ndelangen left a comment

Choose a reason for hiding this comment

Hypnosphi commented Sep 4, 2017

Hypnosphi left a comment

Choose a reason for hiding this comment

shilman commented Sep 5, 2017

Hypnosphi commented Sep 5, 2017 • edited Loading

ndelangen commented Sep 5, 2017

thomasbertet commented Sep 5, 2017 via email

ndelangen commented Sep 5, 2017

thomasbertet commented Sep 6, 2017

ndelangen commented Sep 6, 2017

ndelangen commented Sep 6, 2017

thomasbertet commented Sep 6, 2017

ndelangen commented Sep 6, 2017

thomasbertet commented Aug 31, 2017 •

edited

Loading

codecov bot commented Aug 31, 2017 •

edited

Loading

thomasbertet commented Aug 31, 2017 •

edited

Loading

Hypnosphi commented Aug 31, 2017 •

edited

Loading

SpaceK33z commented Sep 1, 2017 •

edited

Loading

thomasbertet commented Sep 1, 2017 •

edited

Loading

Hypnosphi commented Sep 5, 2017 •

edited

Loading