-
Notifications
You must be signed in to change notification settings - Fork 805
-
Notifications
You must be signed in to change notification settings - Fork 805
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
Workbox 3 module loading may depend on non-spec-compliant 'importScripts(...)' behavior (in some cases) #1504
Comments
Hmmm... so I know @gauntface looked into this when he implementing the lazy-loading-via- In your use case, I think the cleanest approach is just moving the stuff that triggers the lazy-loading outside of the const strategy = new workbox.strategies.CacheFirst({ /* ... */ });
self.addEventListener('fetch', async event => {
const result = await strategy.handle({ event }) || Response.error();
event.respondWith(Promise.resolve(result));
}); It's something we should do a better job of calling out in the docs, especially for folks who go with the roll-your-own-event-handler approach. |
I have tested the above worker in Chrome (before workaround), and it does incorrectly succeed, where in Edge it produces |
Hmmm... weird! In any case, does moving the lazy-loaded-bits outside of the |
I think for the general use case, yes that's a sufficient workaround (moving lazy loaded bits outside fetch handler). Our case is a bit more complex 😄 Basically, we create cache names on the fly (user segregated caches) and to do that, we are calling Placing |
(As an alternative, you can create your own custom builds of Workbox by consuming the ES2015 module source code, and import that single, static build into your top-level service worker.) |
Great idea, I will have to give that a try! We are already using webpack + TypeScript so it might make sense to just consume workbox as an ESM instead of |
CC: @philipwalton who knows more about ESM usage, in case you run into any issues. |
FYI looks like Chrome will align to Spec |
I'm aligning firefox to the spec as well. |
FYI I added a method to load modules up front because I was a little concerned this could crop up:
Not as elegant as it could be - but it should work unless I'm missing something |
@gauntface fantastic! that should work for our case. Feel free to close this issue since support for loading modules on demand outside of the Proxy usage is already supported. |
It might be worth discussing if we should be documenting this better and / or requiring this way or doing things. Generally - I think most users of workbox won't hit this but I've stepped back from the project to up to @philipwalton and @jeffposnick for next steps. |
So what i did was reduce my code to minimal and then test using the suggestions provided // const variables
const workboxCdnUrl = "https://storage.googleapis.com/workbox-cdn/releases/3.2.0/workbox-sw.js";
const cacheName = "sp-cache-v1";
const expirationTimeInSeconds = 60 * 60 * 24;
const CDN_HOST_ENDS_WITH = '.akamaihd.net';
//main async function
async function main(){
var importWorkbox = (cdnUrl) => {
importScripts(cdnUrl);
}
// importing workbox
await importWorkbox(workboxCdnUrl);
// function to enable logging if mode value is debug it reads value from a cache and sets config //accordingly
await (async function switchDebugMode()
{
var cache = await caches.open('sw-mode');;
// var res=await cache.match("https://sw-mode/cache");
// var json=JSON.parse(await res.text());
//currently making it true for testing purpose
if(1===1)
{
// if we want to enable logging for debugging puposes
workbox.setConfig({
debug: true
});
}
})()
// Loading Modules
workbox.loadModule('workbox-core');
workbox.loadModule('workbox-strategies');
workbox.loadModule('workbox-routing');
workbox.loadModule('workbox-cache-expiration');
workbox.loadModule('workbox-cacheable-response');
workbox.core.setLogLevel(workbox.core.LOG_LEVELS.log);
// using stale while revalidate for all urls
workbox.routing.registerRoute(
new RegExp(".*"),
workbox.strategies.staleWhileRevalidate({
cacheName: cacheName
})
);
}
// calling the main function
main().then(console.log("service worker is installed")) In chrome this works fine but in edge it agains throws an error Unable to import module 'workbox-core' from 'https://storage.googleapis.com/workbox-cdn/releases/3.2.0/workbox-core.dev.js'. Am i missing anything |
All of your code is running inside of an There's really no benefit to using that My advice is to:
|
FYI, Chrome is moving forward with disallowing the non-compliant use of I'm going to close this issue and will submit a PR to update https://developers.google.com/web/tools/workbox/guides/get-started#importing_workbox to make it clearer that anything which might dynamically call |
Library Affected:
workbox-sw 3.X
Browser & Platform:
Effect is really only present on MS Edge
Issue or Feature Request Description:
According to the SW Spec:
However, there are cases where workbox (via its
Proxy
) may runimportScripts(...)
during runtime.Here's an example:
To be honest it's more of a Chrome / FF bug then a workbox bug, and maybe the spec should be updated and the browsers should align one way or another, but for workbox to work correctly in Edge, a note in the docs or a descriptive console error on this case would be good.
You could probably
try-catch
and look forNetworkError
, and if the worker state is not'install'
, ask the user toimportScripts(...)
any workbox modules directly duringinstall
if they won't be imported during install by default.So the workaround is:
The text was updated successfully, but these errors were encountered: