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

Webpack Discussion #696

Closed
goldhand opened this issue Jul 22, 2017 · 17 comments
Closed

Webpack Discussion #696

goldhand opened this issue Jul 22, 2017 · 17 comments
Labels
Discuss An open question, where input from the community would be appreciated. workbox-webpack-plugin

Comments

@goldhand
Copy link
Collaborator

Library Affected:
workbox-webpack-plugin

I wanted to discuss the idea of supporting the same configuration from sw-precache-webpack-plugin inside workbox-webpack-plugin.

I think it would make it easier for users of sw-precache-webpack-plugin to transition to using workbox-webpack-plugin if they are able to use the same configuration with workbox-webpack-plugin.

I also wanted to consolidate a list of any other feature suggestions that might help the workbox / webpack integration feel seamless.

A few I'd like to see are:

There's also a lot of requests for webpack-dev-middleware/dev-server support as seen in #577 and goldhand/sw-precache-webpack-plugin#17

@gauntface
Copy link

@goldhand sounds good to me, do you have a list of things that aren't currently supported? Would help me get a feel for what is missing in workbox-build.

@goldhand
Copy link
Collaborator Author

@gauntface

Any sw-precache-webpack-plugin options that were added to support sw-precache

These are found under the plugin options: section of the sw-precache-webpack-plugin configuration docs.

I think workbox-build covers:

    staticFileGlobsIgnorePatterns: 'globIgnores'
    filepath: 'swDest', 'globDirectory'

filename isn't really needed.

which only leaves minify (minifies/uglifies) and mergeStaticsConfig (lets you keep webpack options and specify your own additional options for staticFileGlobs and stripPrefixMulti)

Any default behaviors that sw-precache-webpack-plugin is passing to sw-precache.

I think we should try to provide the same default behavior mapped to the workbox equivalent, if possible.

These are found under the sw-precache options: section of the sw-precache-webpack-plugin configuration docs.

I need to look more into it but my connection is pretty spotty at the moment (sw cache use case 😆) but it looks like currently there is:

  • globDirectory
  • swDest
  • globPatterns

That provide default options for workbox-build. I don't think all the functionality is the same but I looks like some are covered.

A couple missing options that are awesome IMHO are:

  • importScripts support for chunks with [hash] names and dynamically imported chunks. See importScripts example.
  • staticFileGlobs is passed actual emitted bundle paths I think this is more reliable than globs (if a regex doesn't match for example).

Any options that sw-precache provided but workbox-webpack-plugin (through workbox-build) does not.

It looks like sw-lib can support the runtimeCaching options that were passed to sw-toolbox from sw-precache.

Others?

@piehei
Copy link

piehei commented Jul 28, 2017

I would like to be able to add other features (like runtime caching) via workbox-webpack-plugin or have a clear documentation how to do it otherwise (sorry if the documentation is available but I've missed it).

What is unclear to me is that:

  • how to you use workbox-webpack-plugin at the end of your webpack production build script to generate a service-worker.js with the latest static files for caching
  • how to add runtime caching, broadcast cache update, or something else, to the same service-worker.js generated in the webpack build process

This very same question could also be laid out differently:

  • how to add the static file listing for precaching generated by workbox-webpack-plugin to an existing, hand-crafted service-worker.js?

If I make an SW script by hand and configure my desired runtime caching, broadcast channel and whatnot, how can I leverage the static file glob listing for precaching in the very same script using the webpack plugin?

@jeffposnick
Copy link
Contributor

Re: a couple of separate comments:

staticFileGlobs is passed actual emitted bundle paths I think this is more reliable than globs (if a regex doesn't match for example).

+1. This is what I'd expect from a Webpack solution, since Webpack already knows everything it needs to get this right. The staticFileGlobsIgnorePatterns configuration can be carried over and still seems like a good model.

how to add the static file listing for precaching generated by workbox-webpack-plugin to an existing, hand-crafted service-worker.js?

So I think that this is one big difference between what you could do with sw-precache-webpack-plugin and what will now be possible. I think this is how things already work with the currently released version of workbox-webpack-plugin, but if not, then I'd argue it should 😄

  • If there's swSrc set in the workbox-webpack-plugin config, then the plugin effectively is in injectManifest() mode, where it will take the source SW file, including any hand-crafted logic, and just fill in the precache([]) placeholder with the out put of the Webpack build.
  • If there's no swSrc, then we're in generateSW() mode, where an entire new service worker file is generate on the fly. We already support the equivalent runtimeCaching and `navigateFallback options in Workbox.

@addyosmani addyosmani added Discuss An open question, where input from the community would be appreciated. P0 labels Jul 29, 2017
@goldhand
Copy link
Collaborator Author

goldhand commented Jul 29, 2017

Random feature idea: it would be super awesome if we could specify chunks to use for precaching like is possible with html-webpack-plugin (but for adding chunks to bundles):

module.exports = {
  ...
  entry: {
    appshell: [
      'babel-polyfill',
      'src/appshell',
    ],
  },
  plugins: [
    WorkboxPlugin({
    ...
    chunks: ['appshell'],
  }),
  ],
};

If the webpack plugin can move away from using glob (and leverage the emitted webpack assets instead) I think this can possibly replace staticFileGlobsIgnorePatterns in the future. I just don't see a need for glob if we're already aware of everything that's being built.

note: by default just cache all the chunks like html-webpack-plugin and sw-precache-webpack-plugin already do

@goldhand
Copy link
Collaborator Author

@jeffposnick it sounds like we don't need importScripts anymore with injectManifest() mode. Is that correct? You just write your own service worker rather than having it generated and "importing" scripts into it.

@jeffposnick
Copy link
Contributor

For better or worse, there are two "modes" supported by workbox-build:

  • injectManifest mode, which gives you full control over your top-level SW. If you're using this mode, then you could just add in your own importScripts() statements, so it wouldn't make sense to support importScripts as a parameter.
  • generateSW mode, which mimics the old sw-precache behavior and writes out the SW file for you. The old options that sw-precache supported for generating code still make sense in this scenario.

The workbox-webpack-build plugin, as it's currently implemented, determines what mode it's in based on whether there's a swSrc parameter in its config. This is not necessarily as intuitive as it should be, so I think it's an open question whether rewriting the interface to be more explicit would make sense.

@goldhand
Copy link
Collaborator Author

goldhand commented Aug 9, 2017

@anilanar work in #724 addresses some issues with the current webpack plugin interface and prototypes a possible solution to these issues.

Issues that were mentioned in this pr:

The proposed solution is to break the webpack plugin into multiple, feature-specific, plugins. What are the pros and cons of this api? Are there other apis that can solve the mentioned issues while minimizing complexity for developers seeking webpack + workbox integration?

@anilanar
Copy link

anilanar commented Aug 10, 2017

I'd also like to make the following points:

Regarding injectManifest and generateFileManifest

  • injectManifest in general is brittle to work with. Significant number of people in the community uses transpilers/compilers/bundlers that modifies the code in unpredictable ways (because, implementation detail). If service-worker is built using one of them, injectManifest is somewhat annoying.
  • generateFileManifest kinda solves the above point, but not really. If manifest file is imported by a service worker file, service worker will not be updated unless service-worker code is different than before. That is somewhat a weakness in current spec for importScripts (I think they are working on fixing it).
  • A prependManifest would solve this issue.
    • Prepend self.${manifestVariableName} = ${assetManifest}; to service-worker.

Regarding service worker in projects bundled with webpack

  • It is safe (?) to assume that users would like to build service-worker with webpack.
    • In my case, we use esnext features (requires transpilation to es6/7), we share code between main app and service-worker, we have some custom loaders/plugins etc.
  • You cannot build service-worker and the app in a single webpack compilation.
    • They are targeting different environments (webworker/sw vs. web).
  • Thus you must compile service-worker first, then build the app, then inject asset manifest into service-worker that was built in the first step.
  • Ideally this setup should work with webpack watch and dev-server modes, but it's not trivial to setup webpack in such a way to make it happen (build two compilations in watch/dev-server mode, one depends on another).

Regarding separation of webpack plugin into separate plugins

  • I don't have strong feelings about this but I see two possible ways:
    • Have a mode option.
    • Have separate plugins.
  • I don't see much difference between webpack plugins and functions. They are both functions. Then I like the symmetry between workbox-build and workbox-webpack-plugin. I think it is a bias to think that it would be more complicated to have 3 different plugins than to have 1 because webpack/js community had sticked to monolith approaches in the past (over-generalization, arguable); but now that is slowly changing to more focused functions/plugins.
  • Those seeking webpack + workbox integration must be aware of differences between those modes.
    • If they are looking for a quick solution, they will use GenerateSWPlugin. The name of the plugin is somewhat self-explanatory.
    • If they are looking for more complicated solutions:
      • Use GenerateFileManifestPlugin plugin (perhaps rename it to GenerateManifestCodePlugin?). This is somewhat useless unless user somehow handles versioning of service-worker, or somehow injects it into service-worker himself; until importScripts spec gets better.
      • Use GenerateManifestJsonPlugin in combination with one of the other plugins; to generate a JSON and do some static analysis on it.
      • Use InjectManifestPlugin, this is also somewhat tricky to use due to reasons given above.
      • Use PrependManifestPlugin to prepend the manifest into a file.

In regards to making workbox easy to use

  • From experience working with service-worker, it is very dangerous to make an API that seems easy to use: copy/paste some code and voila!
  • I'm working together with smart people, even then we made wrong assumptions about everything. We had bugs with precaches not updating, hashes, revisions, problems with cdn, with varnish cache and bunch of other stuff because we thought it was as easy as adding sw-precache-webpack-plugin into webpack.
  • Handling all edge cases is hard, but there are not too many edge cases. This is something workbox can work on. Handle all edge cases properly, document different use-cases very well and don't tell people that service workers and caches and precaches are simple. Instead tell them that it is hard but workbox comes to their help.

TBH, this is all too complicated in part due to how webpack works and how SW makes some things harder. Nonetheless, I can't think of a quick solution. The problem itself is hard and the solution would involve a combination of many small solutions all around.

BTW I have a quick implementation for prependManifest here: anilanar@8cafb93

@addyosmani
Copy link
Member

addyosmani commented Aug 10, 2017

I'll apologize for being late to this issue with input.

Any sw-precache-webpack-plugin options that were added to support sw-precache

I agree with your earlier suggestions, @goldhand. Support for mergeStaticsConfig for maintaining your Webpack options and providing your own additional ones for staticFileGlobs (etc) sound like a reasonable gap to fill.

Minification is an interesting one. From a purist perspective, I wish we could defer this to the user to decide how best to proceed, but I've seen enough Webpack configs where folks forget (or aren't aware) they need to do extra steps to minify that I think taking care of minify parity is worth considering too.

I think we should try to provide the same default behavior mapped to the workbox equivalent, if possible.

+1. This will be particularly important for our migration story.

staticFileGlobs is passed actual emitted bundle paths I think this is more reliable than globs (if a regex doesn't match for example).

I'll echo Jeff's support for something like this. Hopefully staticFileGlobsIgnorePatterns will be
sufficient here.

Random feature idea: it would be super awesome if we could specify chunks to use for precaching like is possible with html-webpack-plugin (but for adding chunks to bundles):

Having had to deal with the pains of wiring things up manually to html-webpack-plugin before, I'll
give this a strong level of support from me. Caching all the chunks per html-webpack-plugin and sw-precache-webpack-plugin as a default sounds fine.

I'll comment on @anilanar's latest in just a sec separately.

@addyosmani
Copy link
Member

Thanks for all of your input, @anilanar!

Regarding injectManifest and generateFileManifest

I can see where prependManifest could help here, but have concerns about it not providing as much control over where you inject in the file. As you mentioned, there are many other tools in a build pipeline that could be making their own alterations here.

@mikefowler on another thread suggested specifying a regex so you can control where in the
file the manifest gets injected. Would this solve the issue prependManifest was proposed for?

I think it is a bias to think that it would be more complicated to have 3 different plugins than to have 1 because webpack/js community had sticked to monolith approaches in the past (over-generalization, arguable); but now that is slowly changing to more focused functions/plugins.

Do you have an example of a Webpack plugin that's been shifting in that direction? This would be useful for reference. Perhaps the first that comes to mind is html-webpack-plugin. The community have a lot more plugins for it that are pretty granular, but I'm unsure how much the html-webpack-plugin team themselves have been using that model to avoid putting too much into the core plugin itself.

In regards to making workbox easy to use

This is all valuable feedback. As you've acknowledged, we're dealing with an API surrounded with inherent complexity, an immense amount of nuance and lots of foot-guns. Some of this pain, workbox can try to shield from users. Other pains we can try our best to document patterns or recipes around but you're right that SW does make some things much harder.

@anilanar
Copy link

anilanar commented Aug 10, 2017

One more observation I made today in our project:

  • Assets have hash in file names like [name].[chunkhash].js or [name].[contenthash].css.
  • After multiple compilations (with normal mode or watch mode), build directory is filled with duplicate files that have different hashes.
  • Workbox plugin picks them all up and puts them in the manifest (because globs). This is not so harming in dev environment, but somewhat confusing. It is probably dangerous for production/deployment if there are leftovers from previous compilations.

Solution:

  1. Clear build directory after every compilation (or ask users to do so).
  2. Implement a custom solution that takes assets from webpack's compilation stats.

@addyosmani

I can see where prependManifest could help here, but have concerns about it not providing as much control over where you inject in the file.

Are there some use cases where you would like to inject it at a specific location? Prepending it would allow you to use the manifest throughout the service worker. We do something similar in webpack by splitting and prepending runtime/chunk manifest to help achieve long-term hash stability. e.g.

// sw.js
import workbox from 'workbox-sw';

const manifest = self.__file_manifest.map(x => `${self.webpackPublicPath}/${x}`);
workbox.precache(manifest);

Do you have an example of a Webpack plugin that's been shifting in that direction?

I don't have one off top of my head. It may be a misconception I wanted to believe in. I think community is ready for it; nevertheless it is not important. Even if there is merit to splitting plugins, its benefit is insignificant right now.

@goldhand
Copy link
Collaborator Author

@anilanar thank you for all your input.

Regarding injectManifest and generateFileManifest

This is a very valid point. We need the emitted assets to exist before we can generate the manifest, then we need to go in and modify one of these assets using that information. It's a difficult problem. Is there another way of approaching this? I think you suggest that generateFileManifest solves this because it is generating an asset rather than modifying an existing one and prependManifest would go one step further by allowing you to still create a custom service worker that can combined with the asset manifest, is that correct?

Regarding service worker in projects bundled with webpack

This is my understanding too but I think we need to come up with an approach that works with a single compilation. We just can't make running webpack twice a requirement in order to support workbox.

In regards to making workbox easy to use

Service workers are extremely powerful and potential dangerous if not used correctly but I don't think that's a reason to make this api difficult to use.

After multiple compilations (with normal mode or watch mode), build directory is filled with duplicate files that have different hashes.

This can be fixed if we use the emitted asset paths from webpack rather than glob patterns when we create the manifest (what sw-precache-webpack-plugin does currently).

@goldhand
Copy link
Collaborator Author

@addyosmani regarding minification: when we generate a service worker, this happens after the webpack compilation, where most people would include their minify / uglify processes (through webpack plugins). The generated service worker asset is not piped through webpack's plugins and can't be minified / uglified using webpack plugins

@gauntface
Copy link

gauntface commented Sep 15, 2017

I'm pretty much pro everything @goldhand is suggesting and I know it will be a drastic change from where we are today.

Sadly, I'm lacking the WebPack knowledge to really comment more than I like what I'm reading. All I can say is - if you can start creating PR's against the v3 branch for the WebPack plugin, raise issues if you when you hit more specific areas where you'd like to discuss API's approaches, I think we can start moving forward.

I'm generally of the opinion that the injectManifest isn't right call for webpack because it has so much more context.

I think the most immediate decision is how does a developer indicate they want to generate a service worker vs add a manifest to an existing service worker (Which can be done via inlining, injecting or importing - what ever is best for WebPack). Any suggestions?

Regarding API / Option names - Please do keep the current options from current precache plugin to ease transitioning, but would be good to have the option to rename options if there is a better name an then just link up to old values (We are currently doing this with workbox-build at the moment in a few places).

@goldhand
Copy link
Collaborator Author

@gauntface One thing that has been difficult is the pattern where you generate-<config-for-thing>.js then have a write-<thing>.js that populates a template then writes that template to the filesystem (like in write-file-manifest.js and write-sw.js. I'm having to duplicate the "populating" logic and am referencing the lodash templates across modules. Would be nice if there was a intermediate step like generate-<template-for-thing>.js or something in the flow so I could just use that then pass the string to webpack.

@gauntface
Copy link

We have landed and continue to land changes on the webpack plugin thanks to @goldhand's awesome work.

There are a number of smaller issues opened and a number of TODO's in code that should cover the issues raised in this monster thread so I'm going to close this for now.

If there are outstanding issues or topics to discuss please open issues :)

<3 to @goldhand for all of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss An open question, where input from the community would be appreciated. workbox-webpack-plugin
Projects
None yet
Development

No branches or pull requests

6 participants