importScripts() should throw?

[annevk]

I thought the idea was that importScripts() was only available during the installation event as it's a synchronous API. And that outside of that specific context we'd make it throw. It seems sad to allow such a synchronous all the time.

[annevk]

I guess it would have to be allowed always during the first task after starting up, otherwise it's kind of meaningless... But no reason to allow it elsehwere I think.

[h2non]

I think should not be applicable now, since the importScripts built-in function has a legacy behavior since Web Worker and Shared Worker specifications also support it.

IMO exceptions are a layer on control-flow which is too much overrated in general, and in JavaScript engines are not too concerned about this block optimization.
As it's sync, maybe returning a boolean could be fine as well, but in the future it will not be able to support ES6 module import behavior as return

[annevk]

Legacy behavior does not matter for a new environment.

[h2non]

You are considering that Service Worker is a pure new environment? I don't think so.
According to the spec it's obviously a child of Web Worker, described as: the core of this system is an event-driven Web Worker which responds to events dispatched from documents and other sources

I think changing the behavior of the same build-in function just for the Service Worker scope is not a reasonable solution.

[annevk]

It's not a child, it has a completely different lifecycle. It's certainly like other workers, but quite a bit different.

[wanderview]

Well, issue #410 is open to make ServiceWorker not inherit Worker. I still think we should do that to avoid confusion between the two. I think there are many differences in behavior already.

It seems making async importScripts() throw is probably the right thing. While it might break module loading frameworks, those frameworks would not work on ServiceWorker anyway if they depend on async importing. Throwing just makes the error explicit instead of mysteriously breaking when you are offline.

Is this something blink would consider implementing at this point?

[h2non]

I think native Web API consistency and coherency should be followed strictly (probably one of a few aspects in software which purism is justified)

In case that a new feature is required to satisfy new problems in the Service Worker specific scope, a possible solution is about introducing a new built-in function which can satisfy those needs without breaking existent interfaces. Fortunately the Worker scope allows us to be more relaxed about adding new global functions which doesn't affect to the global Window scope, "poisoning" the global scope

[slightlyoff]

I don't want to change the behavior of importScripts beyond the restrictions we're considering for dynamic calls. Async behavior is not on. If you want it, that's what module syntax is for.

[annevk]

To be clear, this bug is not about making importScripts async. It's about restricting it in general since I thought the idea was that service workers would not have synchronous APIs...

[mfalken]

As an implementer I like the idea of allowing it only at startup and oninstall. Blink is starting to implement timeouts for long-running SW in order to conserve CPU/battery and prevent instances of getting "stuck", and we could easily allow startup/oninstall to have more JS execution time than other events.

[jungkees]

Currently, the spec has changed to allow importScript(urls) only in oninstall based on the discussion in this thread and #730.

However, I don't feel like it's a right behavior as even the cached scripts will not have any chance to execute again later when other functional events are dispatched.

As the service worker's imported scripts updated flag already guarantees the network fetch will occur only during the update phase and in the later runs the cached scripts are always used (cache miss is a network error), I suggest we remove this restriction that limits the use only in oninstall.

As an implementer I like the idea of allowing it only at startup

To make it clear, the importScripts in the current spec runs only at startup as Run Service Worker algorithm just returns early when called while it's already running.

[slightlyoff]

I think this works. Thanks for the attention to detail!

[wanderview]

Wait, looking at the current spec I don't see the import scripts allowed flag set during top level worker script evaluation. Is that correct?

It seems we should allow importScripts() to run at the top level in addition to the install event. Otherwise we are going to break a lot of existing service workers out there.

[jungkees]

It seems we should allow importScripts() to run at the top level in addition to the install event.

Yes, this is what I've concerned about too. What I suggested is to remove the step checking the importscripts allowed flag in the importScripts(urls) entirely. Then, for the top level worker script evaluation, importScripts will be allowed to run, and naturally this top level evaluation runs only once as the Run Service Worker algorithm checks whether the worker's already running and returns early there if so. And as such, it seems we don't have to confine the evaluation to oninstall handler as that's just a one time event for certain script version.

[jungkees]

Removed the restriction of allowing it to only oninstall: 319a49e. So now it's allowed anywhere during the top level worker script evaluation, and from the second Run Service Worker that spins off and creates the environment, the imported scripts will be served from the cache.

[wanderview]

@jungkees I think I understand now. Sorry for my confusion.

What about for a service worker being restarted from an existing registration? It seems we don't set "imported scripts updated flag" in that case since we don't run the install algorithm. Right?

[jungkees]

For a specific service worker version, its imported scripts updated flag is set only once during its installation phase when the installation succeeded (Install algorithm step 17). So the service worker being restarted from an existing registration should have been set its imported scripts updated flag through the installation already. So to speak, importScripts(urls) will get a new script resources from the network only when the registration updates with a new version.

[wanderview]

But the imported scrips updated flag is on the ServiceWorker, not the ServiceWorkerRegistration. Are the flags on ServiceWorker supposed to be persisted across browser restarts? I thought only the registration was persistent.

[wanderview]

Ok. I guess they are persistent since the state is on ServiceWorker. My mental model was different from the spec. Sorry for my confusion.

[jungkees]

Ah.. I see. They are persistent as you noted.

Closing.