0.10.0 regression in create-react-app integration

[jeffposnick]

BEFORE YOU SUBMIT please read the following:

• I'm submitting a bug report

webpack version:
2.x

sw-precache-webpack-plugin version:
0.10.0

Please tell us about your environment:
OSX 10.x

We've been working on a create-react-app integration, tracked inhttps://github.com/facebook/create-react-app/pull/1728

facebook/create-react-app#1728 (comment) describes a regression when using v0.10.0+ of this plugin, in which the Webpack build process is run twice.

Looking at the change between v0.9.1 and v0.10.0, I guess the issue might have been triggered by something around here: https://github.com/goldhand/sw-precache-webpack-plugin/compare/v0.9.1...v0.10.0?ws=0#diff-1fdf421c05c1140f6d71444ea2b27638R68

But I don't know enough about Webpack to say for sure.

[hulkish]

@jeffposnick can you try this with v0.11? Also, if i pull that branch used for that pr will i be able to replicate it locally?

[jeffposnick]

Yes, the regression was introduced in v0.10.0, and I've been able to reproduce it in the subsequent versions, up to v0.11.0.

You should be able to reproduce by checking out this commit: https://github.com/jeffposnick/create-react-app/tree/f9b303aa038fedc79adc4ec6e6e94611ef2bae81

Then:

$ npm install && npm run build

[hulkish]

@jeffposnick hmm... I have seen this same issue myself progressively more and more lately. I am wondering if this issue is coming from within webpack itself? I'd have to dig to be sure.

In terms of timeline of the frequency I've seen this issue and how often webpack/sw-precache-webpack-plugin has done releases... It just points more toward webpack as the cause.

I mean, it seems plain as day to me that sw-precache-webpack-plugin isn't the likely cause here. You can see here that we are only doing a single operation once after-emit is applied.

Try cloning my fork here: https://github.com/hulkish/create-react-app/tree/bugtest

I added verbose webpack logging.. the issue now points closer to extract-text-webpack-plugin. For example, this is the very end of the log:

[16:45:19] Webpack (32%) - Build modules
[16:45:19] Webpack (71%) - Optimize modules (sealing)
[16:45:19] Webpack (72%) - Optimize modules (optimizing)
[16:45:19] Webpack (73%) - Optimize modules (basic module optimization)
[16:45:19] Webpack (74%) - Optimize modules (module optimization)
[16:45:19] Webpack (75%) - Optimize modules (advanced module optimization)
[16:45:19] Webpack (76%) - Optimize modules (basic chunk optimization)
[16:45:19] Webpack (77%) - Optimize modules (chunk optimization)
[16:45:19] Webpack (78%) - Optimize modules (advanced chunk optimization)
[16:45:19] Webpack (32%) - Build modules (~/dev/repos/create-react-app/packages/react-scripts/node_modules/extract-text-webpack-plugin/loader.js??ref--4-0!~/dev/repos/create-react-app/packages/react-scripts/node_modules/style-loader/index.js!~/dev/repos/create-react-app/packages/react-scripts/node_modules/css-loader/index.js??ref--4-2!~/dev/repos/create-react-app/packages/react-scripts/node_modules/postcss-loader/lib/index.js??postcss!~/dev/repos/create-react-app/packages/react-scripts/template/src/index.css)
[16:45:19] Webpack (32%) - Build modules (~/dev/repos/create-react-app/packages/react-scripts/node_modules/extract-text-webpack-plugin/loader.js??ref--4-0!~/dev/repos/create-react-app/packages/react-scripts/node_modules/style-loader/index.js!~/dev/repos/create-react-app/packages/react-scripts/node_modules/css-loader/index.js??ref--4-2!~/dev/repos/create-react-app/packages/react-scripts/node_modules/postcss-loader/lib/index.js??postcss!~/dev/repos/create-react-app/packages/react-scripts/template/src/App.css)
[16:45:19] Webpack (33%) - Build modules (~/dev/repos/create-react-app/packages/react-scripts/node_modules/extract-text-webpack-plugin/loader.js??ref--4-0!~/dev/repos/create-react-app/packages/react-scripts/node_modules/style-loader/index.js!~/dev/repos/create-react-app/packages/react-scripts/node_modules/css-loader/index.js??ref--4-2!~/dev/repos/create-react-app/packages/react-scripts/node_modules/postcss-loader/lib/index.js??postcss!~/dev/repos/create-react-app/packages/react-scripts/template/src/App.css)
[16:45:19] Webpack (33%) - Build modules
[16:45:19] Webpack (79%) - Optimize modules (module and chunk tree optimization)
[16:45:19] Webpack (80%) - Optimize modules (module reviving)
[16:45:19] Webpack (81%) - Optimize modules (module order optimization)
[16:45:19] Webpack (82%) - Optimize modules (module id optimization)
[16:45:19] Webpack (83%) - Optimize modules (chunk reviving)
[16:45:19] Webpack (84%) - Optimize modules (chunk order optimization)
[16:45:19] Webpack (85%) - Optimize modules (chunk id optimization)
[16:45:19] Webpack (86%) - Optimize modules (hashing)
[16:45:19] Webpack (87%) - Optimize modules (module assets processing)
[16:45:19] Webpack (88%) - Optimize modules (chunk assets processing)
[16:45:19] Webpack (89%) - Optimize modules (additional chunk assets processing)
[16:45:19] Webpack (90%) - Optimize modules (recording)
[16:45:19] Webpack (91%) - Optimize modules (additional asset processing)
[16:45:19] Webpack (92%) - Optimize modules (chunk asset optimization)
[16:45:23] Webpack (94%) - Optimize modules (asset optimization)
[16:45:23] Webpack (95%) - Emit files
Total precache size is about 161 kB for 4 resources.

[16:45:24] Webpack: Finished after 8.456 seconds.

[hulkish]

On another note.... I wonder if this could be solved in sw-precache-webpack-plugin by listening for emit instead of after-emit. Thoughts @goldhand ?

[goldhand]

The only thing I can think of is that I started using webpack's built in writeFile instead of nodejs fs here (because of #64).

Tomorrow morning I'll try switching to using fs again and also test @hulkish theory about emit/after-emit. I'll keep digging into this issue until I find a fix because it looks like its blocking the facebook/create-react-app#1728 pr.

Thanks for being so involved @hulkish, if you find anything or have any other ideas keep us posted 😄

[jeffposnick]

Re: create-react-app, the PR landed using v0.9.1. So it's not blocking anything, and thanks again for everyone's hard work in maintaining this plugin. It should be getting a significant uptick in usage soon 😄

[goldhand]

I'm having trouble reproducing the two webpack builds error discovered by @Timer facebook/create-react-app#1728 (comment)

I tried with / without dynamic chunks but still can't reproduce it with 0.11.0

[goldhand]

I think we want this latest version to be in cra btw, it supports webpack chunks.

[goldhand]

I think @hulkish is correct, this doesn't seem like a sw-precache-webpack-plugin issue. I have not been able to reproduce it yet.

[hulkish]

@goldhand actually, I've been able to reproduce it... be sure to check out the particular commit he mentioned:

git clone https://github.com/jeffposnick/create-react-app.git
cd create-react-app
git checkout f9b303aa038fedc79adc4ec6e6e94611ef2bae81

Then, you can see the effects of duplicated output by commenting/uncommenting the usage of sw-precache-wepack-plugin (which lives in packages/react-scripts)

While I agree that this points to this plugin as the cause, sure... However, I'm not quite convinced just yet.

[goldhand]

Ty @hulkish, I was using the facebook/create-react-app master branch. I don't see that problem there btw. Will test with jeffposnick's repo though

[hulkish]

@goldhand I'm actually working on a pretty big pr for you.. I think that if we are the cause of this issue... then my approach should fix that.

My pr will bring:

• complete support of the webpack compiler instance's inputFileSystem & outputFileSystem :

  • all the way down to the swPrecache.generate() level.
  • this includes all third party modules which require('fs')
  • It does this by processing the swPrecache.generate() in a node vm. This allows me to override and mock the require of the fs module.
  • This means it'll support webpack-dev-server + hot module reloading!! 🎉

• I've changed the processing strategy for this plugin to one very similar to how html-webpack-plugin solves this:

  1. ln SWPrecachePlugin.prototype.apply I listen for the compiler.plugin('make') event.This is after webpack has already walked through your code and acquired all resource paths. But, before the compilation has begun. This allows us to run in parallel with the parent compilation.
  2. In the handler for this event i create a child compiler.
  3. in parallel to the main compilation by tapping into the parent/main compilation's make lifecycle event: This means faster overall compilation!
  4. on emit i then record all asset output to the compilation.

• Deprecated minify option

  • Since output assets are now properly recorded to the active webpack compilation.assets/chunks, the output will be minified by any included minification plugin to your webpack config.
  • For example: new webpack.optimize.UglifyJsPlugin()
  • This will still work, to avoid a breaking change. However, I've installed a warning to report when you're using the minify option. Then maybe next minor or major release removes this option entirely.

• Split a bit of logic into plugins of their own, which then get applied from the main plugin's apply()

• added a bunch of tests

[goldhand]

That is awesome @hulkish, Winning!

[goldhand]

How's it coming @hulkish? I'm really excited to see the node vm part of your solution 😄

[jeffposnick]

Thanks to @goldhand and especially @hulkish for diving into this!

I think it's probably too late in the sw-precache development lifecycle to fundamentally change the way it handles manifest generation, so apologies that the monkey-patches are necessary.

That being said, the current workbox-webpack-plugin (roughly equivalent to what sw-precache-webpack-plugin is trying to achieve, but for the new Workbox set of libraries) is a very lightweight wrapper right now. If you think the approach that you're taking for sw-precache-webpack-plugin is generally useful, then perhaps there's some willingness to port it forward to workbox-webpack-plugin?

CC: @prateekbh, who's been developing workbox-webpack-plugin on our team. See also GoogleChrome/workbox#269, which includes an early design discussion.

[hulkish]

@goldhand @jeffposnick so, the vm solution will work but i kind of hate it, lol.

It's really just a way to work around what se-precache doesnt expose independently (fs to support memoryfilesystem.

The real challenge I'm finding is the reliability of this approach. It's a fairly fragile approachbased on implementation detail of sw-precache. Which could have really annoying impact on supporting future versions of sw-precache.

For example... its not quite so simple as to run it in a VM because there is various other internals of sw-precache that generally make it a very awkward fit to the webpack plugin system.

So.. here's what i want to do instead:

1. Rewrite sw-precache with a webpack-first approach.

   • This involves basically just isolating and exposing the options defaulting/processing & lodash template compiling. (I've actually already done most of this... but wanted to get but in from you peeps before going too far)

2. First class support for webpack-dev-server, HMR & strictly using inputFileSystem/outputFileSystem for all source/asset input/output.

3. Borrowing quite a bit from html-webpack-plugin's approach

Why?

• When it comes down to it... we really only need one thing from sw-precache: the lodash template.
• A part from processing options etc... Every single thing else that sw-precache is doing within it's generate method is undesired from a webpack perspective.
• For example.. they use glob.sync() to locate staticGlobPatterns. Just a small example of a roadblock for something that actually isnt needed at all on the webpack side of things.
• I am convinced it's worth the effort because right now the way the service worker is generated is very black boxed.
• Still early to boast i guess..but unless a giant anvil falls in the code somehow this approach is going to be way faster.

Other notes...

• While this obviously would be a breaking change.. I intend to keep the whole purpose of sw-precache to remain the same.
• This will really open this plugin up a bit more to allow it to integrate.more smoothly all around.
• I'm happy to first submit my sw-precache rewrite as a pr there...but i have doubts they'll fully convert to my approach.
• I believe it's probably best to cut the direct sw-precache dependency and move forward with the essentials. (template..sw-toolbox etc)

@goldhand im sure you've ran into some of thia before as well. what you think?

[goldhand]

Thanks for the update @hulkish, I will continue to think on this. I've been down the rabbit hole a couple times and haven't been able to find a working solution for webpack-dev-server + sw-precache so I was really interested to see how the vm solution would work.

Maybe create branch that uses https://github.com/GoogleChrome/workbox instead of sw-precache (as opposed to rewriting sw-precache). As @jeffposnick mentioned, it will be a lot easier to get your ideas into workbox than sw-precache.

I don't think we can re-write sw-precache, and I'm not sure we should remove it from this plugin (considering it's called sw-precache-webpack-plugin) but if we can get webpack-dev-server working with all the sw-precache benefits, I suppose we could try it 😄

I would like to get a fix for create-react-app though and release it in 0.11.1 so create-react-app can have chunk support. Any luck figuring out the issue with the duplicate build?

[hulkish]

@goldhand ur probably right. I actually am not very familiar with workbox at all. But, it does look a bit more plausible per my concerns.

However, it still doesn't support a dependency injected file system. They are still using fs directly.

As far as the duplicate webpack output goes.. I think this is because we are emitting assets after the compile has already completed via the after-emit plugin event. I think webpack recognizes this then does an additional pass as the default behavior. for when outputFileSystem.writeFile.

For now, im thinking ill go back to my solution of using vm just to at least make it work with webpack-dev-server. Ill let you know if that changes.

[prateekbh]

@hulkish if you are interested i'll be happy to give u hand with workbox, or if u wanna comeback and do it after you are done with sw-precache

[jeffposnick]

Workbox's precaching logic assumes that it will be passed an array containing a "manifest" of URLs. The manifest entries can either be an Object, in which case both the url and revision fields should be present, or a String, which is assumed to be a URL that already includes revisioning information via a hash in the filename. E.g.:

[
  {url: '/path/to/index.html', revision: '<hash_of_index.html>'},
  '/path/to/versioned/file.<hash>.js',
  // ...etc.
]

It's up to the build tool to generate an appropriate manifest corresponding to the current assets and "inject" it into a well-known placeholder in a source service worker file:

const workboxSW = new WorkboxSW();

// The source file contains an empty array as a placeholder:
workboxSW.precache([]);

// After the build process has completed, the final file has [] replaced with the manifest:
// workboxSW.precache([/*actual manifest*/]);

The process of generating the manifest is up to the build tool.

We shipped Workbox v1.0.0 with some tools that follow the sw-precache model and use glob patterns to read from the filesystem, but if it makes sense to do something different for a Webpack plugin, then that's a legitimate use case.

Ideally, the options that the Webpack plugin supported would be roughly equivalent to what other Workbox build tools supported, but if it made sense not to support, e.g., globPatterns and instead just infer the assets based on the output of the Webpack build, (along with a provision to white/blacklist certain assets), that sounds reasonable to me.

[goldhand]

@jeffposnick, if I understand correctly, we can generate a manifest (array of filepaths) and provide that to workboxSW.prechache(manifest).

Is glob not used then, if we provide the manifest?

As @hulkish mentioned, there is some mismatch with sw-precache and webpack coming from sw-precache's reliance on glob.

Webpack knows the all the filepaths (and revisions) so that feature in sw-precache is a hinderance (for usage with webpack).

If we can generate the manifest (and avoid the usage of glob) that would really be awesome!

[jeffposnick]

Basically, yes.

If you have a URL that already is versioned (because it has a hash in its URL thanks to Webpack), then you can just pass that string in as an entry in the array. URLs that can't be versioned (like most HTML documents) need to be specified in the {url: '/path/to/index.html', revision: '<hash_of_index.html>'} format, rather than as a string.

workbox-build is just one approach to generating this manifest. As long as the manifest adheres to that format I've described, it can be created via some other process (like by listening to the output of a Webpack build).

The actual process of spitting out a final service-worker.js file that has proper references to the workbox-sw runtime library and has the manifest in the right place (i.e. workboxSW.precache(<manifest>)) is the other piece of the puzzle, but that's largely a matter of templating.