Details

Reviewers

atul
tomek
kamil
ashoat

Commits

rCOMMecd1e3be96da: [keyserver] Serve web worker files

Summary

Part of ENG-2628

We need to serve web/service worker files from the keyserver because during registration we need to pass a url where it's available. So the web app would call e.g. serviceWorkers.register('http://web.comm.app/workers/notif') to register the notification service worker. Service workers work only in a specific "scope" which depends on the url, but we want the notif service worker to apply everywhere and the simplest way is to just set the Service-Worker-Allowed header.

Depends on D6715

Test Plan

Test that the file is correctly served in dev and prod modes. Test with later diffs if the service worker is correctly registered

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Feb 13 2023, 7:08 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 13 2023, 7:08 AM

Harbormaster completed remote builds in B16395: Diff 22432.Feb 13 2023, 7:34 AM

michal requested review of this revision.Feb 13 2023, 7:34 AM

Did you test this in dev and prod modes? Is there a way to server a hot-reloading version of this bundle? Does the bundle really contain CommonJS (hence the .cjs extension)?

michal mentioned this in D6715: [web] Add support for web workers to webpack.Feb 14 2023, 7:10 AM

kamil added inline comments.Feb 15 2023, 3:28 AM

keyserver/src/responders/webworker-responders.js
6 ↗	(On Diff #22432)	I think in all files for responders we have functions definition and then at the bottom of the file `export {` instead of inline. Also, maybe the return type will be better for clarity.
10 ↗	(On Diff #22432)	Shouldn't we return a different status code when the `worker` param is not defined at all? (`400` or `422` comes to mind but not sure)
15 ↗	(On Diff #22432)	Probably I'll be better to leave the default scope. If I understand correctly notifs feature will not have to intercept calls to the network, so there is no need for broad scope - that being said we can have only one SW per scope and if we will want to introduce SW in the future which will intercept network calls we will need to move this one (which seems more complicated than just changing scopes). Looks like the default scope, without the header or specifying sth like `'Service-Worker-Allowed': '/push-notifs', and then registering SW in the same scope on the client will be sufficient.

michal planned changes to this revision.Feb 15 2023, 3:32 AM

michal edited the test plan for this revision. (Show Details)Feb 15 2023, 4:46 AM

Changed formatting, remove Service-Worker-Allowed header (it's actually not needed if we don't specify a custom scope during registration)

@ashoat:

Tested on prod mode and amended the test plan
Hot-reloading was already discussed in D6715
From what I understand the default target for webpack is CommonJS so .cjs should be correct.

keyserver/src/responders/webworker-responders.js
10 ↗	(On Diff #22432)	In express route parameters can't be null or undefined. So currently it will just redirect to our `'*'` route returns the web app. We could probably handle it with additional routes for `'worker'` and `'worker/'` but I'm not sure if that's worth it.

From what I understand the default target for webpack is CommonJS so .cjs should be correct.

I'm a bit confused here. commonjs2 isn't an option for target, but it is an option for libraryTarget and library.type. But those only appear to be relevant if the output is a library, and I don't think our output is.

It makes sense that it's only relevant if the output is a library... CommonJS tells us how module 1 will include (require / import) module 2, but in this case we don't have a module loading it.

Can you please actually open the file that you've suffixed with .cjs, check the contents, and see if there is anything "CommonJS" about it?

This revision now requires changes to proceed.Feb 15 2023, 7:42 AM

Harbormaster failed remote builds in B16531: Diff 22616!Feb 15 2023, 10:54 AM

I'm a bit confused here. commonjs2 isn't an option for target [...]

Sorry, I shouldn't have used the word "target", I meant this option: output.module. In the generated file I can see some webpack boilerplate that defines properties on the exports object so the file should be cjs.

Sorry, I shouldn't have used the word "target", I meant this option: output.module.

Are you sure? The docs say:

output.module is an experimental feature and can only be enabled by setting experiments.outputModule to true.

We don't seem to enable that in D6715.

In the generated file I can see some webpack boilerplate that defines properties on the exports object so the file should be cjs.

This is good to hear, and a more compelling argument. But that might just indicate that the modules *within* the bundle are being bundled together via CommonJS. Is there a global variable named exports that is set?

My expectation would be that this is not being compiled as a "module" at all, eg. nobody is going to import this bundle... the bundle is just going to be run directly by the browser. It would be surprising to me if the browser was looking at exports.

If that's correct, and there is no global exports set, then the suffix should probably be .js and not .cjs.

There's no global exports set and "the bundle is just going to be run directly by the browser" is correct, so I renamed the file to .js.
Additionaly I'm bringing back the Service-Worker-Allowed header because (now that I understand it better) this service worker should have all tabs in scope and needs to be able to claim them all (for example to focus on them after clicking the notification). It's not like it handles only some part of the website.

Harbormaster failed remote builds in B16612: Diff 22718!Feb 20 2023, 8:10 AM

Please rebase on master, re-run yarn cleaninstall, update this diff, and confirm all CI gates are passing before landing!

for example to focus on them after clicking the notification

If possible, I think it would be best if only the tab that is being brought to focus navigates to the notification. The rest of them should stay where they are

This revision is now accepted and ready to land.Feb 20 2023, 2:22 PM

Rebase

If possible, I think it would be best if only the tab that is being brought to focus navigates to the notification. The rest of them should stay where they are

Yeah, that's how it's going to work

Harbormaster completed remote builds in B16684: Diff 22817.Feb 21 2023, 3:27 AM

michal added a child revision: D6817: [keyserver] Send web push notification.Feb 24 2023, 4:06 AM

Closed by commit rCOMMecd1e3be96da: [keyserver] Serve web worker files (authored by Michal Gniadek <michal.gniadek@swmansion.com>). · Explain WhyMar 1 2023, 12:50 AM

This revision was automatically updated to reflect the committed changes.

Michal Gniadek <michal.gniadek@swmansion.com> added a commit: rCOMMecd1e3be96da: [keyserver] Serve web worker files.