surma/rollup-plugin-off-main-thread

importScripts() inside a Promise + SW event handlers in Firefox 74

jeffposnick opened this issue · 4 comments

jeffposnick commented

If you use rollup-plugin-off-main-thread while bundling a service worker script, and one of the files that is pulled in via importScripts() inside of a Promise ends up calling addEventListener('fetch', ...), it triggers a Fetch event handlers must be added during the worker script’s initial evaluation error message in Firefox 74.

You can reproduce this by clearing out previous service worker registrations and then visiting https://proxx.app/ or https://jeffy.info/ in Firefox.

This was previously discussed at GoogleChromeLabs/proxx#470 (in which @jakearchibald said it shouldn't be an issue), but recently came up again in Workbox's issue tracker at GoogleChrome/workbox#2448

As far as I can tell, this error message doesn't affect functionality (i.e. the fetch handlers added via the imported scripts still appear to work), so this might just be inaccurate error reporting in Firefox.

Even if this behavior is technically allowed by the SW spec, I wonder if it's possible for rollup-plugin-off-main-thread to ship an optional loader template that's tailored to the SW importScripts() use case, in which the entire import flow isn't wrapped in a Promise? I've tried making that change myself in a custom template, but never got very far, because I wasn't sure how it would fit in with the require + registry logic. I feel like it might be the sort of thing that would take me days to figure out, and take someone familiar with the codebase significantly less time.

CC: @wanderview @philipwalton

wanderview commented

@asutherland for current firefox behavior.

asutherland commented

This is a Firefox bug, thank you for bringing it to our attention! It is now filed as https://bugzilla.mozilla.org/show_bug.cgi?id=1629125

The key things to note are:

  • The error message is wrong.
  • Jake is right.
  • Any "fetch" handler added in the microtask checkpoint immediately following the top-level script evaluation will correctly mark the ServiceWorker as handling "fetch" events despite the error message.
jeffposnick commented

Thanks for the confirmation, @asutherland!

(I'd still love to figure out how to create a loader template for rollup-plugin-off-main-thread that resulted in importScripts() that wasn't wrapped in a Promise for the service worker use case, but that's definitely not a priority as long as things work.)

jakearchibald commented

I haven't really read or understood this issue, but "Jake is right" has made my Easter weekend. Thank you.