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 injecting manifests into existing webpack-compiled assets #1765

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@pimterry

pimterry commented Nov 22, 2018

R: @jeffposnick @philipwalton

Fixes #1513

Description of what's changed/fixed:

Before this PR, the only way to use InjectManifest was to inject into files totally outside the rest of your webpack compilation process. As in #1513, this causes problems (in my case, it means it's impossible to use InjectManifest in TypeScript).

With this PR, there's now two ways to configure InjectManifest:

  • As before: setting swSrc to the path of an input file, independent of webpack, and optionally using swDest to override the output file.
  • New: setting only swDest, to specify an existing output file that you'd like to inject into.

This means if my build process already builds a service-worker.js file somehow (e.g. via TypeScript), I can now set up workbox by adding the plugin with just:

new InjectManifest({
    swDest: 'update-workers.js'
});

I considered creating a third parameter instead (swExisting?) so you could inject and use swDest as well. I don't think there's any case where you'll want to inject into an existing file and then have both files end up in your output though. I'm fairly sure you always want to totally overwrite an output file, and that's simpler this way. Let me know if you disagree.

I've written this such that all existing configs should keep working fine, and updated the validation in line with these changes to catch any issues. The new behaviour uses ConcatSource instead of bare strings, so it doesn't break source maps etc too. I wasn't sure how to update the docs corresponding to this, so I've left those for now.

@pimterry

This comment has been minimized.

pimterry commented Nov 23, 2018

Build is failing here due to incorrect hashes in the produced manifest filenames. I'm not sure why though, and I can't seem to reproduce it at all locally with gulp test. Any idea what I'm missing?

It'd be easy to just update the test hashes so that travis will pass, but presumably that'd then break locally instead, and it doesn't seem sensible to do that without knowing why this is failing.

@pimterry

This comment has been minimized.

pimterry commented Nov 23, 2018

In the meantime I need this for my build, so I've published this forked version to npm as @httptoolkit/workbox-webpack-plugin (version 3.6.4-pr1765.1). Might be useful for testing or for anybody else with the same issue.

@jeffposnick

This comment has been minimized.

Collaborator

jeffposnick commented Dec 10, 2018

Thanks for the contribution, @pimterry!

We've roped @developit into working on a larger rethinking of the workbox-webpack-plugin interface. I'd like to get his thoughts about this approach, though the fact that it's backwards-compatible does make me more inclined to give the go-ahead.

A few logistical issues: we are (really!) going to launch v4.0.0 from the next branch at some point in the near future. If this PR were to land before we launched, we'd want to get it rebased against next. That should also work around the Travis CI failures, as the Travis config has been adjusted on next to lock down the expected version of Node, which is the underlying issue. Don't worry about that for now—I can rebase for you if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment