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

importWorkboxFrom: 'local' bypasses the webpack asset pipeline #1338

Closed
jeffposnick opened this issue Mar 1, 2018 · 6 comments
Closed

importWorkboxFrom: 'local' bypasses the webpack asset pipeline #1338

jeffposnick opened this issue Mar 1, 2018 · 6 comments

Comments

@jeffposnick
Copy link
Contributor

Library Affected:
workbox-webpack-plugin

Moving over @raejin's comment into a standalone issue:

After testing out v3.0.0-beta.1, I think this is not compatible with static asset that gets served from a different domain. By tracing the source a bit, we are copying the file on the disk instead of copying the file to the Webpack compilation.

https://github.com/raejin/workbox/blob/17c38de990585d7076b03ecca55d33b3166e53e9/packages/workbox-build/src/lib/copy-workbox-libraries.js#L78-L79

What do you guys think of a different implementation on copying on disk versus Webpack compilation?

@jeffposnick
Copy link
Contributor Author

You're correct that we end up copying files using the "real" filesystem rather than adding the files to the webpack compilation. There's a discussion of this in the codebase:

case 'local': {
// This will create a local copy of the Workbox runtime libraries in
// the output directory, independent of the webpack build pipeline.
// In general, this should work, but one thing to keep in mind is that
// when using the webpack-dev-server, the output will be created on
// disk, rather than in the in-memory filesystem. (webpack-dev-server will
// still be able to serve the runtime libraries from disk.)
const wbDir = await copyWorkboxLibraries(compilation.options.output.path);
const workboxSWImport = (compilation.options.output.publicPath || '')
+ wbDir + '/workbox-sw.js';
// We need to set this extra option in the config to ensure that the
// workbox library loader knows where to get the local libraries from.
config.modulePathPrefix = wbDir;
return [workboxSWImport];
}

The alternative is not being able to share that copyWorkboxLibraries() logic from workbox-build and implementing something equivalent that just works for webpack. That's possible, but I want to understand exactly how this is breaking compatibility for your setup.

The code currently takes compilation.options.output.publicPath into account when coming up with the URL used for the import, which I was under the impression was the "standard" way of specifying "custom" serving URLs. Can you explain more about what constraints you have?

Also, given that we're close to the v3.0.0 release, it would be good to understand how much of a "blocker" you consider this issue.

@jeffposnick jeffposnick added the Needs More Info Waiting on additional information from the community. label Mar 1, 2018
@raejin
Copy link

raejin commented Mar 1, 2018

@jeffposnick

I should have clarified that this behavior will make it inconsistent in development, specifically for our Webpack setup which relies on a separate Webpack dev server (which will serve the asset from a separate subdomain).

Here is a simple illustration of what the structure looks like:

├── app // all the source code for JS
└── public
    ├── service-worker.js // on disk, served from a non Webpack dev server domain
    └── webpackFolder
        ├── precache-manifest.js  // In development, this doesn't live on disk, which will served from a separate domain
        ├── workbox-generated-service-worker-script.js // In development, this doesn't live on disk, which will served from a separate domain
        └── workbox-v3.0.0-beta.1 // on disk, served from a non Webpack dev server domain

This is not a blocker at all provided that the solution definitely works in production. Prior to beta.1, I already had a solution that copies on behalf of Webpack asset pipeline for development && production consistency.

Thank you for looking into this!

@jeffposnick jeffposnick added enhancement and removed Needs More Info Waiting on additional information from the community. labels Mar 1, 2018
@jeffposnick
Copy link
Contributor Author

Thanks for the additional information, @raejin.

Given that, I'm not going to consider this within scope for the upcoming v3 release, but will leave this issue open to track future refactoring to use the webpack asset pipeline instead of the local filesystem when in importWorkboxFrom: 'local' mode.

@madmoizo
Copy link

In 3.1.0 and dev only, precache-manifest is copied in importsDirectory but workbox-v3.1.0 is not

    new InjectManifest({
      swSrc: `${rootPath}/src/sw.js`,
      importWorkboxFrom: "local",
      importsDirectory: "static/lib/workbox",
      include: [/\.html$/, /\.js$/, /\.css$/, /\.woff2$/]
    })

webpack 4.5.0
webpack-serve 0.3.0

@jeffposnick
Copy link
Contributor Author

In Workbox v5, any assets created by GenerateSW are added directly to compilation.assets and rather than written to the file system.

(We also generate a single, custom runtime bundle of the Workbox code you actually use, rather than copy over the full set of Workbox libraries to the output directory.)

@jeffposnick
Copy link
Contributor Author

This should be addressed by the current Workbox v5.0.0 alpha.

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

No branches or pull requests

3 participants