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

Allow including files just before manager.bundle.js #1134

Merged
merged 12 commits into from Jun 7, 2017

Conversation

enjoylife
Copy link
Contributor

What I did

I simply followed the preexisting approach of including additional html, but instead of it being injected into the story iframe, it is injected in the index.html, right before the manager script.

This commit really was just to satisfy my issue detailed below, but the pull request is more an example for the maintainers to help solve a few outstanding issues/requests. See #678 , #686, #630.

Context & Why?

The process went like this:

  1. Why are storybook's build times so slow (5 sec or more)?
  2. Lets try adding the webpack dll plugin
  3. Oh wait, now I need to include a vendor.js (it has the react bits) prior to storybook
  4. This commit
  5. Now build times are sub 200ms.

How to test

Include a file called bodyscript.html in the storybook config directory, and you will see it be injected into the source.

Context & Why?
The process went like this:
1. Why are storybook's build times so slow (5 sec or more)?
2. Lets try adding the webpack dll plugin
3. Oh wait, now I need to include a vendor.js (it has the react bits) prior to storybook
4. This commit
5. Now build times are sub 200ms.
@enjoylife enjoylife changed the title Allow including a js files just before manager.bundle.js Allow including files just before manager.bundle.js May 26, 2017
@enjoylife enjoylife force-pushed the master branch 2 times, most recently from 49bc32e to b390c34 Compare May 26, 2017 20:01
@codecov
Copy link

codecov bot commented May 26, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@31dba14). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1134   +/-   ##
=========================================
  Coverage          ?   12.73%           
=========================================
  Files             ?      199           
  Lines             ?     4577           
  Branches          ?      725           
=========================================
  Hits              ?      583           
  Misses            ?     3352           
  Partials          ?      642
Impacted Files Coverage Δ
app/react/src/server/utils.js 21.73% <0%> (ø)
app/react/src/server/middleware.js 0% <0%> (ø)
app/react/src/server/index.html.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 31dba14...468515b. Read the comment docs.

@ndelangen
Copy link
Member

ndelangen commented May 26, 2017

Thanks for this PR!!! is this related to #1083 you think?..

Would it be an option to use DLLplugin by default? I haven't used it before, So I could use help with that.

@enjoylife
Copy link
Contributor Author

From my limited knowledge (reading an hour and this blog post), with the DLL plugin, its a two step thing.

  1. You have a separate webpack config which does just does the bundling of your "vendor" packages upfront.
    webpack --config .\.storybook\vender.webpack.config.js
//  .storybook/vendor.webpack.config.js

var path = require("path");
var webpack = require("webpack");

module.exports = {
  context: process.cwd(),
  entry: {
    vendor:[
      'react',
      'react-dom',
      'react-router'
       // any other "expensive" packages to build
      ]
  },

  output: {
    filename: '[name].dll.js',
    path: path.join(__dirname,'public'),
    library: '[name]',
  },

  plugins: [
    new webpack.DllPlugin({
      path: path.join(__dirname,'public', "[name]-manifest.json"),
      name: "[name]",
      context: path.resolve(__dirname)
    })
  ]
};

That outputs two files

public\vendor.dll.js
public\vendor-manifest.json

Then in the webpack config being used by storybook, we incorporate the run time part which doesn't output chunks but reads in the generated manifest

// .storybook/webpack.config.js
// ...
 plugins: [
    // Use the DLL in development.
    new webpack.DllReferencePlugin({
      context: __dirname,
      manifest: require("./public/vendor-manifest.json"),
    }) 
  ]
// ...

Then any chunks at runtime will be expecting the pregenerated vendor script to be loaded prior to their execution.

 <script src="vendor.dll.js"></script>
 <script src="static/manager.bundle.js"></script>

So to answer one of your questions, It is related to #1083, but instead of dynamic chunks, its more about needing an additional explicit configurable list of public js files

@ndelangen
Copy link
Member

Right, that confirms what I thought it was. This will definitely be supported one way or another!

Thank you so much for this crazy good PR! ❤️

@shilman
Copy link
Member

shilman commented May 27, 2017

"Now build times are sub 200ms."

Drops mic.

This is amazing. Thanks for this wonderful PR. 🙇

@ndelangen
Copy link
Member

@enjoylife I'm going to investigate adding / using the DLL plugin to speed up the builds for everyone!

concerning this PR. What I think I'd like to do merge this, with a simple change.

Right now your proposal is to add support for a bodyscript.html file.
We already have a head.html file. As a consumer, I'd be really confused what file has what purpose.. What do you think of this suggestion:

  • rename head.html to preview-head.html *
  • add support for a new file called manager-head.html

(*) if this file would not be found we would look for head.html for backwards compatibility.
We could display a deprecation warning if head.html was provided, asking the user to rename it.

@ndelangen ndelangen modified the milestones: v3.1.0, v3.0.1 May 27, 2017
Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

Right now your proposal is to add support for a bodyscript.html file.
We already have a head.html file. As a consumer, I'd be really confused what file has what purpose.. What do you think of this suggestion:

  • rename head.html to preview-head.html *
  • add support for a new file called manager-head.html

(*) if this file would not be found we would look for head.html for backwards compatibility.
We could display a deprecation warning if head.html was provided, asking the user to rename it.

@enjoylife
Copy link
Contributor Author

The original names in the pull request were hastily thought out... so if those changes better align with the existing naming conventions in storybook, then that is great!

@ndelangen
Copy link
Member

I'm can accept this PR if you change the filenames (with support for legacy head.html.

Bonuspoints for adding this behavior to the docs! ❤️

@ndelangen
Copy link
Member

@enjoylife Are you interested in updating this PR? Is there something I can do to help?

@enjoylife
Copy link
Contributor Author

@ndelangen, I updated the naming conventions as per our previous comments and added a few updates to the docs.

@ndelangen ndelangen modified the milestones: v3.0.2, v3.1.0 Jun 7, 2017
@ndelangen ndelangen merged commit 468515b into storybookjs:master Jun 7, 2017
@shilman
Copy link
Member

shilman commented Jun 9, 2017

@ndelangen I think you are doing something funky with git here, and it is causing the merge commit to be improperly labeled, which is breaking my release notes generator. Please merge PR's from the github website unless you have a very good reason not to. For now I'll add this one by hand.

storybook_ _less_ _git_log_ _89x43

@@ -70,6 +70,7 @@ export default function(data) {
background-color: #eee
}
</style>
${headHtml}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is buggy when headHtml is not defined

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

3 participants