search

GoogleChromeLabs / proxx

A game of proximity
https://proxx.app
Apache License 2.0
1.31k stars 124 forks source link

Issue with SW async install (& importScripts)? #470

Open jeffposnick opened 5 years ago

jeffposnick commented 5 years ago

Debug info

Screen Shot 2019-05-29 at 10 01 22 AM

Describe the bug (I'm not sure how much of an issue this actually is, and I'd be interested in hearing from @jakearchibald in particular here.)

I was planning on using rollup-plugin-loadz0r in a project of my own, and was curious as to how Proxx addressed an issue I first reported at https://github.com/surma/rollup-plugin-loadz0r/issues/3. Here's the summary:

As per the SW spec, and summarized at https://github.com/w3c/ServiceWorker/issues/1319#issuecomment-393386106, calls to importScripts() are allowed either during the initial, synchronous, top-level SW execution or during the install event handler.

From what I can tell by looking at the final, bundled Proxx service worker, the install event listener doesn't get registered until after importScripts() is called, and both importScripts() and then the install listener registration happens asynchronously, following a promise resolution.

My understanding was that both of those things were problematic to do asynchronously, and instead had to be done during the initial, synchronous execution of the service worker script.

To Reproduce There's inherently a race condition here, and I haven't been able to reproduce a failure, but in theory, the following could happen:

  1. Visit Proxx without an existing service worker registration.
  2. The importScripts() and install event listener happen following a promise resolution, which seems like it could cause issues in certain browsers, as per the service worker specification.

Expected behavior The safest thing (to my mind, at least, but correct me if I'm wrong) would be if rollup-plugin-loadz0r had a mode in which it detected that you were in the ServiceWorkerGlobalScope and instead of implementing an asynchronous define, instead just implemented a synchronous defined that was effectively an alias for importScripts().

jakearchibald commented 5 years ago

There isn't a race condition here, as there's a microtask checkpoint before the has ever been evaluated flag is set.

When spec land calls JavaScript, microtasks will always happen before the next line in the spec, due to the clean up after running script steps.

Regardless, I can't decide if loadz0r should be sync in workers, or if it should be 'async' for consistency. My gut reaction is that it should be sync, but I don't have any good reasoning.

surma commented 5 years ago

Yeah, it should probably be sync. I need to update it for some breaking changes from rollup anyway.

Until then: We ship our own loadz0r template anyways, so we can fix it there. I’ll tackle that in the next sprint.