Use webpack to (re)generate index HTML files #839

toolness · 2015-04-29T15:16:30Z

While researching #585 I ran into jlongster's Backend Apps with Webpack series of posts and decided to familiarize myself with using webpack in node (as opposed to the browser) by using it to watch and rebuild our static index files during gulp watch.

This has a number of benefits:

Rebuilding index HTML files is a lot faster. We don't have to spawn a subprocess anymore, and webpack rebuilds only the code that's changed.
Index HTML files are only rebuilt when an actual file they use changes. Currently, for example, changing lib/travis.js will trigger a full rebuild of the index HTML files, even though that file has nothing to do with rebuilding index HTML files.
We can reuse more of our webpack code pipeline in node, rather than relying on a completely separate build toolchain (in the form of node-jsx). We haven't completely decoupled ourselves from that toolchain yet, as everything outside of gulp watch still uses it, but the plan is to migrate away from it, so that our build toolchain is ultimately simplified. ~~We're still using node-jsx but only for running node-side tests.~~ This should also make migrating to Use a lightweight server that dynamically renders HTML #585 (and quickly iterating on the server during development) easier.

Limitations:

We're not rebuilding the sitemap.xml file in gulp watch anymore. Hopefully not that big a deal.
~~Until we've completely replaced our use of node-jsx with webpack, we're moving away from dev/prod parity, which isn't awesome. So we should finish that migration ASAP.~~
Running the test/index-static.test.js test is slower because it builds the webpack bundle now, but I'd rather it just run slower than assume the developer manually builds it before running the test, and then have devs get frustrated when they change the code but the tests still fail.
We're building the index HTML generator bundle in dist/index-static, which probably isn't awesome because it's really just an intermediate file that isn't intended for distribution. As such, we should probably put it in some sort of intermediate directory that's also in our .gitignore. The index HTML generator bundle is in the build directory, intended for intermediate build files rather than files to be distributed.

Conflicts: lib/index-static.jsx lib/routes.jsx

toolness · 2015-04-30T12:55:29Z

@mmmavis do you want to take a look at this one? It's a bit of a doozy so no worries if not, but I think you could learn a lot from it. This stuff is also pretty new to me, so I could use a second opinion :)

If you accept the mission, I recommend trying out the following to manually test things:

While running gulp watch, intentionally add syntax and runtime errors in various places in the code. This is to simulate real developers making real editing mistakes. Then make sure the console logs useful errors!
While running gulp watch, add basic edits in various parts of the code, and make sure the site regerates itself properly.
Run gulp without this PR, make a copy of the dist directory, then run gulp with this PR, and make sure the index.html files in the two dist directories are identical.

toolness · 2015-04-30T13:03:13Z

gulpfile.js

@@ -70,6 +70,32 @@ function handleError() {
  });
 }

+function createIndexFileStream() {
+  var stream = new PassThrough({ objectMode: true });


This returns an IndexFileStream of all the index.html files in the site. It does a bit of trickery because we first need to generate the webpack bundle that generates the index files.

mmmavis · 2015-05-06T16:15:06Z

@toolness I'll get to this PR today - hopefully I won't take too long this time 😶

toolness · 2015-05-06T17:37:52Z

No worries! It's not a big rush either, so feel free to take your time and ask any questions that come up.

mmmavis · 2015-05-11T17:37:01Z

Sorry for the delay - my other tickets took longer than I expected. Will work on this one today 😁

mmmavis · 2015-05-12T18:59:37Z

lib/index-static-watcher.js

+    entry: entry,
+    target: 'node',
+    devtool: 'sourcemap',
+    externals: function(context, request, callback) {


Hey @toolness could you help me understand what context and request are? I see they being mentioned on http://webpack.github.io/docs/configuration.html#externals but the doc doesn't go in detail.

Ah yeah, this was annoying for me to figure out, because webpack's documentation is sparse in some places. :( I think I literally just console.log'd the arguments to get an idea of what they actually were, lol. Not a very strong sign of forwards-compatibility, but the good news is that at least we'll know when it breaks, since that code is executed so often!

heh, console.log is always good! I think it'll help a lot for understanding webpack related stuff especially... 😶

Yeah, webpack can be really confusing. :( Hopefully it will improve with age...

mmmavis · 2015-05-13T04:48:10Z

Hi @toolness

While running gulp watch, intentionally add syntax and runtime errors in various places in the code. This is to simulate real developers making real editing mistakes. Then make sure the console logs useful errors!

Working perfectly.

While running gulp watch, add basic edits in various parts of the code, and make sure the site regerates itself properly.

Yes, this works great as well. (files got regenerated themselves and with manual refresh on browser I was able to see the changes)

Run gulp without this PR, make a copy of the dist directory, then run gulp with this PR, and make sure the index.html files in the two dist directories are identical.

Question: by identical did you mean comparing the code in those files or?

mmmavis · 2015-05-13T06:20:50Z

lib/gulp-index-file-stream.js

+  }, function(err, html) {
+    if (err) {
+      return this.emit('error', err);
+    }


(commenting on this line instead since Github doesn't let me do it on line 19)

I'm just curious why we have to reverse the array. Does it matter which url gets popped first? Is it to preserve the ordering that the URLs get set in indexStatic.URLS?

Ah, yeah, it is to preserve the ordering. Instead of using .pop() in the _read method I should've just used .shift(), but because I constantly get confused between shift and unshift and didn't want to look up the docs, at the time I just opted to reverse the array being popped-from instead! 😅

toolness · 2015-05-13T11:57:43Z

Question: by identical did you mean comparing the code in those files or?

Yeah, um, I really should've done this myself before issuing this PR, but I think it'd be useful for both of us to do it, just to make sure. Basically I mean e.g. using the current develop branch without this PR and generating the files, then renaming the dist directory to dist-old, then switching to this PR and generating the files, and comparing the index.html files in dist and dist-old using a tool like diff. Since theoretically, both should be identical.

As per the discussion at: mozilla#839 (comment)

mmmavis · 2015-05-14T18:17:47Z

lib/index-static-watcher.js

+    }
+  };
+
+  self.build = function(cb) {


Hi @toolness , I just wanna check I'm getting this right.

So this part means
if index-static.bundle.js has been generated, move on
if it's still being build, we make sure it doesn't get rebuilt within 100ms?

Very close--basically we don't want multiple builds going on simultaneously, so we just block any additional requests after the first one to wait until the first one is finished. They "wait" by coming back every 100ms and checking to see if the first one is done yet.

It's kind of an overly-complicated way to do it honestly. Maybe I should use a singleton promise instead?

mmmavis · 2015-05-14T19:07:19Z

Hey @toolness could you rebase this PR with develop so I can diff comparing the 2 copies of dist? thanks!

mmmavis · 2015-05-14T19:44:52Z

I rebased on my local branch but diff dist/ dist-develop/ did return some code differences back. 😮 I don't always trust my rebasing skill so @toolness could you rebase and double check?

toolness · 2015-05-14T21:18:10Z

Ugh, yeah, actually don't worry about diffing the files--even doing this kind of diff on the exact same branch appears to fail, because the data-reactid being given to elements is totally changing between runs. It must be based on the current timestamp or something? Anyways, super annoying.

mmmavis · 2015-05-14T21:38:59Z

Alrighty! This PR totally make sense to me now! Thanks for walking me through @toolness
Hooray R+ 👍 and I can finally unblock you 😂

👯 👯 👯 👯 👯

Use webpack to (re)generate index HTML files

toolness · 2015-05-14T22:16:48Z

Wooot thanks mavis!

toolness added 9 commits April 28, 2015 20:32

attempt to use webpack to generate static site.

435e846

use require() to reload index-static, not vm.

0b2916b

move index watching code to lib/index-static-watcher.js.

865cfee

add source map support to index-static bundle.

9ecd861

fix style warnings.

4be513f

use gutil.log instead of console.log.

717422a

update node-jsx, only transform .jsx files.

b74dbb6

add fault tolerance for rebuilding index files.

ad9480a

Merge branch 'develop' into webpackify-index-static

e0d9efe

Conflicts: lib/index-static.jsx lib/routes.jsx

toolness added this to the The Wiz milestone Apr 29, 2015

toolness added 4 commits April 29, 2015 14:37

turn IndexStaticWatcher into a class.

feed6ff

use webpack bundle to generate index files for production too.

746320d

get rid of node-jsx entirely.

138ea42

simplify/streamline static bundle builder thingy.

df4034e

toolness changed the title ~~[WIP] Use webpack to watch and regenerate index HTML files~~ [WIP] Use webpack to (re)generate index HTML files Apr 29, 2015

toolness added 4 commits April 30, 2015 08:07

add some docs to index-static-watcher.js.

37b3e14

remove irrelevant, outdated comment.

b336dc6

don't write same files into dist dir simultaneously.

7618d1b

don't compile the same webpack bundle simultaneously.

109e65c

toolness reviewed Apr 30, 2015
View reviewed changes

toolness changed the title ~~[WIP] Use webpack to (re)generate index HTML files~~ Use webpack to (re)generate index HTML files Apr 30, 2015

toolness added the PR needs review label Apr 30, 2015

toolness added 2 commits April 30, 2015 17:33

streamline calculation of webpack externals.

a33a2bd

put index-static bundle in 'build', not 'dist'.

41bbb67

mmmavis self-assigned this May 6, 2015

hannahkane added this to the Optimistic 80s Movie milestone May 11, 2015

hannahkane removed this from the The Wiz milestone May 11, 2015

mmmavis reviewed May 12, 2015
View reviewed changes

mmmavis reviewed May 13, 2015
View reviewed changes

Add comment about draining stream during test-react-warnings.

a2a0ec1

As per the discussion at: mozilla#839 (comment)

toolness mentioned this pull request May 14, 2015

Add Open Graph meta tags #798

Closed

mmmavis reviewed May 14, 2015
View reviewed changes

mmmavis added a commit that referenced this pull request May 14, 2015

Merge pull request #839 from toolness/webpackify-index-static

950565b

Use webpack to (re)generate index HTML files

mmmavis merged commit 950565b into mozilla:develop May 14, 2015

toolness mentioned this pull request May 22, 2015

Initial attempt at a lightweight dynamic server #936

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use webpack to (re)generate index HTML files #839

Use webpack to (re)generate index HTML files #839

toolness commented Apr 29, 2015

toolness commented Apr 30, 2015

toolness Apr 30, 2015

mmmavis commented May 6, 2015

toolness commented May 6, 2015

mmmavis commented May 11, 2015

mmmavis May 12, 2015

toolness May 12, 2015

mmmavis May 12, 2015

toolness May 12, 2015

mmmavis commented May 13, 2015

mmmavis May 13, 2015

toolness May 13, 2015

toolness commented May 13, 2015

mmmavis May 14, 2015

toolness May 14, 2015

mmmavis commented May 14, 2015

mmmavis commented May 14, 2015

toolness commented May 14, 2015

mmmavis commented May 14, 2015

toolness commented May 14, 2015

Use webpack to (re)generate index HTML files #839

Use webpack to (re)generate index HTML files #839

Conversation

toolness commented Apr 29, 2015

toolness commented Apr 30, 2015

Choose a reason for hiding this comment

mmmavis commented May 6, 2015

toolness commented May 6, 2015

mmmavis commented May 11, 2015

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

mmmavis commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toolness commented May 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmmavis commented May 14, 2015

mmmavis commented May 14, 2015

toolness commented May 14, 2015

mmmavis commented May 14, 2015

toolness commented May 14, 2015