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

Clarify dynamic-imported scripts are never installed #1356

Closed
d0iasm opened this issue Sep 26, 2018 · 33 comments
Closed

Clarify dynamic-imported scripts are never installed #1356

d0iasm opened this issue Sep 26, 2018 · 33 comments
Assignees
Labels

Comments

@d0iasm
Copy link

d0iasm commented Sep 26, 2018

Current spec seems to have no description about how to handle dynamic-imported scripts.

We have 3 situations to install scripts:

  1. Classic script + importScripts()
  2. Module script + static import
  3. Classic/Module script + dynamic import

In 1., a top-level script and all imported scripts are installed because we must guarantee that all scripts for ServiceWorker exist for offline support. Note that importScripts() is allowed to be called only on the initial script evaluation or the install event. See for details #1319 and Intent to Deprecate and Remove: importScripts() of new scripts after service worker installation.

In 2., a top-level script and all imported scripts are installed because of the same reason of 1..

However, in 3., we cannot ensure all imported scripts exist on installation because a service worker could use import() after installation.

My suggestion is we should clarify that a top-level script and static-imported scripts are installed but all dynamic-imported scripts are NOT installed in the spec because dynamic-import is a kind of networking APIs to fetch subresources such as fetch() or XMLHttpRequest that are never installed. If users want to use scripts offline, they should import them with static import. Or, they should explicitly store them in a storage like CacheStorage and dynamically import them as Data URL etc.

@wanderview
Copy link
Member

Can we make dynamic import() throw after the install event completes like we do for importScripts()?

My impression is no one has implemented or shipped es modules on service workers yet, so hopefully we have a lot of leeway to make changes here.

@domenic
Copy link
Contributor

domenic commented Sep 26, 2018

It seems bad to make dynamic import() throw. Unlike importScripts(), it's not sync, so it's not problematic in that regard of blocking requests. It would just drive people to use fetch() + eval() hacks if they want to lazy-load, or conditionally load, any code.

@wanderview
Copy link
Member

It would just drive people to use fetch() + eval() hacks if they want to lazy-load, or conditionally load, any code.

I seem to recall the driving motivation was to avoid people building service workers that functioned when online and then unexpectedly broke offline. The sync'ness of importScripts was not really a factor. We wanted to remove the footgun by fast failing.

@annevk
Copy link
Member

annevk commented Sep 28, 2018

I think it was in part the synchronous nature as it would effectively block the event loop.

@wanderview
Copy link
Member

I think it was in part the synchronous nature as it would effectively block the event loop.

Ok. I guess I don't recall that.

I don't think the fact import() is asynchronous addresses the concerns about sites using it accidentally after install when the resource won't be offlined. We explicitly tried to remove that footgun for importScripts and it feels weird to add it back for import().

Or are you saying import() resources should be offlined whenever its used?

I guess someone needs to actually write a PR to make import() do anything offline in service workers. AFAICT there is nothing in the service worker spec which says it should use offline resources. For example, there is nothing that says import() should use or modify script resource map like we do for importScripts here:

https://w3c.github.io/ServiceWorker/#importscripts

Of course, maybe I don't understand the module stuff since I don't see where Run Service Worker uses script resource map for loading static module imports either:

https://w3c.github.io/ServiceWorker/#run-service-worker-algorithm

Sorry if I'm just confused.

@annevk
Copy link
Member

annevk commented Sep 28, 2018

What I was thinking was that if you wanted to use import() you'd have to cache those resources in advance for the offline case (if it's important offline). (I don't really know the answers to all your questions though. Hopefully @domenic and @jungkees do.)

@domenic
Copy link
Contributor

domenic commented Sep 28, 2018

Yes, I think import() should be treated symmetrically to fetch(), most likely. Both are ways of dynamically fetching resources at runtime, which will behave in predictable ways (e.g. fail unless cached) when offline.

@asutherland
Copy link

asutherland commented Sep 28, 2018

Yes, I think import() should be treated symmetrically to fetch()

I'm confused what this means here. fetch() in a SW bypasses the SW and has no awareness of the script resource map. import() explicitly takes a string right now per the spec, and we disabled URL.createObjectURL() in ServiceWorkers. If the user is offline, it seems like there's no way for a dynamic import to succeed. This seems like a footgun by definition.

@mfalken
Copy link
Member

mfalken commented Oct 26, 2018

F2F: There was some discussion of options:

  • Match static imports.
  • Make dynamic imports just like fetch(): no installation, no bouncing through FetchEvent
  • Allow fetches of dynamic imports to go to the service worker's own FetchEvent to enable offline.
  • Just throw on dynamic imports.

It turned out Chrome is the only browser with immediate plans to implement module service workers.

There was rough consensus that throwing on dynamic imports for now is the safe thing to do, with room to change later as we get real usage of module service workers and understand the problem more.

Todo:

  • Spec that static module imports are installed
  • Spec that dynamic module imports throw
  • Add tests.

@domenic
Copy link
Contributor

domenic commented Oct 26, 2018

Note that import() works in classic scripts (including classic script service workers), in theory. Is the consensus to block it even in that case?

@mfalken
Copy link
Member

mfalken commented Oct 26, 2018

We didn't discuss that, and I didn't know that. But yes, I think we'd block dynamic imports from ServiceWorkerGlobalScope regardless of whether it's a module or classic service worker.

@nhiroki
Copy link
Contributor

nhiroki commented Oct 30, 2018

Chromium issue: https://bugs.chromium.org/p/chromium/issues/detail?id=900045

@philipwalton
Copy link
Member

There was rough consensus that throwing on dynamic imports for now is the safe thing to do, with room to change later as we get real usage of module service workers and understand the problem more.

Why not just try and throw if the network request fails? (keeping in mind all the existing limitations for loading scripts async in SW via importScripts()).

From a developer perspective, the primary use case I can think of for dynamic import() in SW would be in the install event -- where you might want to lazily load migration code based on the user's current version and the new installing version. If you have many previous versions, you wouldn't want all users to have to load all migration code for all previous versions every time the SW starts up.

But in the install event, you're most likely to have network, so it's unlikely to be an issue.

The other concern I have about always failing is it'd make it much harder to generically share code between the window and the SW. Realistically, though, I can't think of many cases where the code I'd want to share would require dynamic import().

@mfalken
Copy link
Member

mfalken commented Jan 17, 2019

Why not just try and throw if the network request fails?

It sounds like this is option: "Make dynamic imports just like fetch(): no installation, no bouncing through FetchEvent".

The cons with that included:

  • Doesn't match static imports (which install the script) which may be surprising.
  • Makes it easy to write service workers that only function while online.

@philipwalton
Copy link
Member

It sounds like this is option: "Make dynamic imports just like fetch(): no installation, no bouncing through FetchEvent".

Oh, I wasn't meaning to imply that I didn't want dynamic import to go through a SW FetchEvent. I think I'd prefer that behavior (even though it is a bit weird).

My reasoning is that I'd expect the same response if I dynamic-imported a URL from both the window and the SW (which I imagine may be somewhat common in the future, e.g. an IndexedDB helper library). If the SW responded with something different, I think that'd be unexpected.

That being said, I can see how you have the potential to get into an infinite loop if the FetchEvent logic can use a dynamic import. Maybe disallow dynamic import in a FetchEvent?

@developit
Copy link

Would it be possible to disallow dynamic import fetching triggered from FetchEvent, but allow dynamic imports that resolve to already-instantiated modules?

@jakearchibald
Copy link
Contributor

Seems possible, but what does that solve?

@davidmaxwaterman
Copy link

I'm sorry to piggy back on this, but I'm having trouble with service worker (workbox) when offline - but I'm using fetch() and import() from the main UI thread (I wondered if one or two of the comments on this issue are thinking they are being called in UI thread, while this is, I think, talking specifically about them being called from the service worker itself).
My issue is that fetch(file) works just fine and I can see workbox messages saying it is serving the file from cache, but import(file) fails with an exception, and no messages from workbox. I first run the app online and it caches the files (I can see them listed in the caches in dev tools), and I then go offline (network panel) and reload the app...and the import() then fails.
Can someone tell me where I could best get some help with this?

@hugo306

This comment has been minimized.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jan 29, 2021

If we're saying that import() should reject until we can figure out the right thing to do with it, should this also reject:

import('data:text/javascript;charset=utf-8,export default 1');

There's no network dependency here, but it feels like rejecting is right, as it provides a feature detect for the rest of import() in service worker.

@annevk
Copy link
Member

annevk commented Jan 29, 2021

Yeah, we should expand the check for Worklet to also cover service workers in https://html.spec.whatwg.org/#hostimportmoduledynamically(referencingscriptormodule,-specifier,-promisecapability). That would suffice I think.

@jakearchibald
Copy link
Contributor

Ohhh yes! I didn't realise that worklets have a similar behaviour here. That makes things a lot simpler.

@jakearchibald
Copy link
Contributor

jakearchibald commented Jan 29, 2021

https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/import-module-scripts.https.html - looks like we need to update this so failure is expected. We should also test:

  • Data url imports
  • Importing something that's already statically imported

It isn't clear to me how the latter should behave, but we should match what currently happens in worklets.

Edit: It rejects before the module registry is checked, so importing something already statically imported will fail.

@jimmywarting
Copy link

jimmywarting commented Feb 20, 2021

#developer-pain

Like Hugo i have also recently adopted a fetch + eval using shimport - hate to use eval but that's what you got to do.
Sometimes if feels like you don't have a choice when you have external dependencies that use dynamic imports and you have no way of changing it unless you fork the hole dependency tree and replace everything with static import or rather importScript. or bundle the hole thing

feels unnecessary to boot up the hole application when only parts of the service worker is being used. It also makes the hole thing "slower" when you have to initiate everything

Not being able to write cross platform independent code for main thread nodejs and service worker and deno is a developer pain

Would be grate if "Allow fetches of dynamic imports to go to the service worker's own FetchEvent to enable offline."
I'm not really concerned about having it working offline. in this modern world we are connected all the time. And static CDN assets have a grate cache header anyway. But to make it work offline it would be grate if it could go throught regular FetchEvent so you can respond with something from CacheStorage or from the sandboxed FileSystem api.

Beside service worker are not only made for offline experiences, ppl build service worker for other reasons that otherwise require a internet connection anyway. You are required to have them if you want to subscribe to web push, use it as an alternative for BroadCast channel or like a shared worker where safari never decided to implement it. Modify network request, like adding authentication header to a <img> that can only do basic GET request. Handle the payment api, emulating what a server dose when it needs to save something large to the disc using streams + respondWith.

I would be fine by not having dynamically import files never installed, and that it's my responsibility to cache it if i really want to make it work offline. And that i have a understanding that i can't attach event listener afterwards. I want to have more control, I would like to be able to progressively enhance the offline experience by putting things into the cache api when necessary.

@jakearchibald
Copy link
Contributor

HTML PR whatwg/html#6395

@jakearchibald
Copy link
Contributor

@jimmywarting we're not throwing out import() forever, we're just making sure we have a route forward that's like's to be backwards compatible. It isn't as simple as it is in other contexts, due to the (reasonable) expectations set by importScripts().

in this modern world we are connected all the time

That isn't true, not even in my experience as a westerner living in a built-up area. Also, 'connected' isn't a binary in practice.

domenic pushed a commit to whatwg/html that referenced this issue Feb 22, 2021
@annevk annevk closed this as completed Feb 24, 2021
jakearchibald added a commit to web-platform-tests/wpt that referenced this issue Feb 24, 2021
@hugo306
Copy link

hugo306 commented Feb 25, 2021

At first I was very confused by this thread.
Why, what appears as a simple request for clarification quickly reached an impasse.
Why the common sense solution seemed to just slide off every time, despite being the suggested solution in the issue itself.
How the circular reasoning about "footguns" got started.
Why deviating behavior was even being considered, despite import() having established behavior now in four other execution contexts. Before wasting your time posting "Hey! Wouldn't it be great if the thing did the thing?" as I did, there is context that's not apparent here.

importScripts() is a pre-serviceWorker pre-Promise legacy API, which was given magical behavior to hack around the ServiceWorker life-cycle, and to prevent naive use from harming performance. It was in the process of causing real-world compatibility problems for ServiceWorker.
importScripts() seems superficially comparable to import(), like a sync version of the same thing. But of course it isn't.
The programmatic loader API has no side-effects. They want to give it side-effects. Such as modifying the cache and internal flags, including it in the byte-for-byte comparison, even making it part of the module-worker-script-graph despite the spec considering them separate graphs.

This API would not do what expect at all. It would basically be a different API with the same name.

However the proposals threw up a bunch of hard contradictions. Breaking offline support. Introducing compatibility risks. Forcing drastic limits to the scopes which allow import().

Having this run down the clock and not be implemented was comparatively a good outcome.

@jakearchibald
Copy link
Contributor

import()'s behaviour shouldn't seem weird compared to static imports, in terms of auto caching, so we'd need to figure that out.

Fwiw, worklets also block import().

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Mar 15, 2021
…estonly

Automatic update from web-platform-tests
No dynamic import in service worker (#27699)

Part of whatwg/html#6395 and w3c/ServiceWorker#1356
--

wpt-commits: a02460e35145989c6fe2365f03d1b0006f381d9d
wpt-pr: 27699
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Mar 15, 2021
…estonly

Automatic update from web-platform-tests
No dynamic import in service worker (#27699)

Part of whatwg/html#6395 and w3c/ServiceWorker#1356
--

wpt-commits: a02460e35145989c6fe2365f03d1b0006f381d9d
wpt-pr: 27699

UltraBlame original commit: 4f89a146b69c4b14730d2418cbac4447c6ac461a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Mar 15, 2021
…estonly

Automatic update from web-platform-tests
No dynamic import in service worker (#27699)

Part of whatwg/html#6395 and w3c/ServiceWorker#1356
--

wpt-commits: a02460e35145989c6fe2365f03d1b0006f381d9d
wpt-pr: 27699

UltraBlame original commit: 4f89a146b69c4b14730d2418cbac4447c6ac461a
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Mar 15, 2021
…estonly

Automatic update from web-platform-tests
No dynamic import in service worker (#27699)

Part of whatwg/html#6395 and w3c/ServiceWorker#1356
--

wpt-commits: a02460e35145989c6fe2365f03d1b0006f381d9d
wpt-pr: 27699

UltraBlame original commit: 4f89a146b69c4b14730d2418cbac4447c6ac461a
@jimmywarting
Copy link

jimmywarting commented Mar 5, 2022

hmm, stumbled upon an issue today that i'm trying to work my way around,
i'm trying out the new type=module in service worker today in chrome and it's awesome 👍

But today i got stuck with "top-level await is not supported in chrome".

I know that your goal of all is to make it all work offline no matter what.
And you should only be allowed to register listeners on installation, and never later, bla bla bla

Even doe I disagree with you on some points... cuz all they is are pain points.

You could always implement your own event emitting algoritm on top of the of an another listener to be able to circumvent this lazy registration.

const ET = new EventTarget
self.onfetch = () => {
  const evt = new Event('fetch')
  ET.dispatchEvent(evt)
}

// sometime later
setTimeout(() => {
  ET.addEventListener('fetch', () => { ... })
}, 1000)

or create some sort of pipeline

const middlewares = [ before, handler, after ]
self.onfetch = evt => {
  evt.respondWith(async () => {
    let response
    for await (let pipe of pipeline) {
      response = pipe(response) 
    }
    return response
  })
}

// sometime later - add a prehook to the middleware array
setTimeout(() => {
  middlewares.unshift(fn)
}, 1000)

So I don't see any reason to block this functionality of lazy event registration when developers can circumvent this anyway in some way or the other that is more a painful experiences.
why i'm i bringing this up? well, it isn't my original issue with top level await that i was faced with today... I haven't read the spec from top to bottom of how it currently work (bad lazy jimmy) but maybe it could have something to do with how you register event listener on installation.

if i'm importing something in a "sync" manner with top level import statement, and one of the files use top level await to acquire something from IndexedDB that it's dependent upon then i think that should be allowed as well

i rather want to acquire something once, then having to do it every time in a fetch event

const cacheStorage = await caches.open('v1')
const authKey = await idb.get('key')
/** @type {FileSystemDirectoryHandle} */
const rootHandle = await idb.get('root')

self.onfetch = evt => {...}

over this pattern:

self.onfetch = evt => {
  evt.respondWith(async () => {
    const cacheStorage = await caches.open('v1')
    ...
  })
}

and there is going to be cases where i wish to conditionally load a polyfill if something isn't supported so again, being forced to import something that isn't needed (b/c it has to be fetched and work offline) with top level import statment is just a waste of bandwidth and time, i rather want to be able to do:

var polyfills = []
if (!Array.prototype.groupBy) polyfills.push(import('./array-prototype-groupBy.js'))
if (!Array.prototype.at) polyfills.push(import('./array-prototype-at.js'))

await Promise.all(polyfills)

vs

import './array-prototype-groupBy.js'
import './array-prototype-at.js'

today i'm building something that should be working offline and requires heavy use of service worker + whatwg/fs to function.
so i always need to get the handle from the indexeddb... no way around that. that's why i wanted to have top level await

My point is: I think ServiceWorker are unnecessary heavenly restricted to what a normal Web Worker can do.
This restriction is actually causing my Service Worker to behave worse as if i would have support for top-level await and also dynamic lazy import(path).

"By preventing developer shooting them self in the left foot then they shot themself in the right foot instead..."

@lemanschik
Copy link

doing now all fetch related stuff in sharedWorkers using service worker only to emit events to the functional sharedWorker solved it for me.

@groogiam
Copy link

@lemanschik Would it be possible to provide some more details on your use of shared workers in regards to handling the fetches. Namely how you are able to access the shared workers from your service workers. I'm trying to dispatch some fetch events to a wasm module that I can no longer load due to the dynamic import restriction and this seems like it might be a viable work around for me. Thanks.

@lemanschik
Copy link

@groogiam BroadcastChannels API is my go to solution for that as this is a Shared Message bus that also works cross tabs and windows as long as the domain is the same https://developer.mozilla.org/en-US/docs/Web/API/Broadcast_Channel_API

@groogiam
Copy link

@lemanschik Thanks for the quick reply and clueing me into the BroadcastChannels API. It is working wonderfully.

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

No branches or pull requests