Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Webpack config refactor; CSS now generated with HTML #253

Merged
merged 4 commits into from
May 8, 2016
Merged

Webpack config refactor; CSS now generated with HTML #253

merged 4 commits into from
May 8, 2016

Conversation

scottnonnenberg
Copy link
Contributor

I realized that the noProductionJavascript option wasn't quite right because it still generates a weird unminified public/bundle.js, and you get no generated styles.css. This pull request changes that.

First, it makes the three stages in util/webpack.config.js a little easier to understand, renaming them to:

  1. develop
  2. build-static
  3. build-spa

Second, it moves styles.css generation to Stage 2, the 'build-static' stage.

Third, it renames the temp webpack-generated file required by static-site-generator-webpack-plugin (previously public/bundle.js, so it was overwritten by the final webpack stage) to public/render-page.js and deletes it when Stage 2 is complete.

@KyleAMathews
Copy link
Contributor

Hi! nice refactor 👍. A few naming suggestions to run by you. What I've been thinking for renaming the stages to is build-css, build-html, and build-javascript. I want to keep css + html building separate for #145 which will add a nice perf boost. Also build-spa is misleading because the JS is not just for eliminating reloads but also for any JS enhancements you add to your react.js components...

An idea to explore in a future refactor (or current if you feel like trying it) would be to see if build times are sped up by spawning a separate process to build the Javascript and CSS/HTML (CSS needs to be built before HTML so it can be inlined).

Because webpack will always generate a final javascript file, the
'build-css' stage isn't as clean as I'd like it.
@scottnonnenberg
Copy link
Contributor Author

Four stages. No use of the term 'SPA.' :0)

I'm not fully happy with the change, however, because webpack doesn't seem to be able to just scan for CSS then produce one resultant CSS file. It will always generate a final bundle.js. So, like the 'build-html' step we have to delete a file afterwards.

That extra bundle.js generation makes the build process quite a bit slower. Here's a full run on my blog with the four stages:

Generating static HTML/CSS
Compiling production bundle.js
Copying assets
Done

real    0m26.855s
user    0m27.204s
sys 0m1.272s

And here's a run on the previous commit, still three stages:

Generating static HTML/CSS
Compiling production bundle.js
Copying assets
Done

real    0m18.992s
user    0m19.291s
sys 0m0.934s

@KyleAMathews
Copy link
Contributor

Hrrm... that is quite a bit slower. I'm really curious now if spawning separate processes would speed things up now. Parallelizing things seems like the only way to eke out more speed.

@KyleAMathews
Copy link
Contributor

I'll try to play with this more soon. Also this is a breaking change so probably we should wait for a few more breaking changes before merging.

@scottnonnenberg
Copy link
Contributor Author

Oh, interesting - I didn't consider it breaking. What's the interface you're considering for the break?

@KyleAMathews
Copy link
Contributor

Anyone modifying the default Webpack config and looking at the stage names.

@scottnonnenberg
Copy link
Contributor Author

Ah yes, there is that way to hook in. Thanks for the reminder. :0)

@KyleAMathews
Copy link
Contributor

I'm going to merge this for now. The ~33% speed decrease we can fix later by just building the bundle.js in another process (https://github.com/sindresorhus/execa looks nice).

@KyleAMathews
Copy link
Contributor

Which will actually probably speed up build times.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants