Page MenuHomePhabricator

[web-db] add Shared Worker config
ClosedPublic

Authored by kamil on Mar 7 2023, 8:31 AM.
Tags
None
Referenced Files
F3352485: D6987.diff
Sat, Nov 23, 6:07 AM
Unknown Object (File)
Fri, Nov 8, 5:33 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Sat, Nov 2, 12:28 AM
Unknown Object (File)
Oct 21 2024, 3:59 PM
Unknown Object (File)
Oct 21 2024, 2:03 AM
Unknown Object (File)
Oct 21 2024, 2:03 AM
Subscribers

Details

Summary

Adding config and file for shared worker.

Test Plan

Run this code:

const webDbWorker = new SharedWorker('/worker/webDatabase');
webDbWorker.port.onmessage = function (e) {
  console.log('message from worker: ', e.data);
};
webDbWorker.port.postMessage('hello there from main thread');

and check results in browser, desktop and shared worker consoles.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Mar 7 2023, 10:05 AM
web/types/worker-types.js
3–5 ↗(On Diff #23505)

I'm surprised this type works... the second part of the union is an object that contains only only property (props), so it is likely incompatible with MessageEvent. Does my inline edit work? Or perhaps even better (but less likely to work):

export type SharedWorkerMessageEvent = {
  ...MessageEvent,
  +ports: $ReadOnlyArray<MessagePort>,
};
keyserver/src/responders/webworker-responders.js
16 ↗(On Diff #23505)

I'm wondering if it is beneficial to have web as a prefix because it all lives in web directory. Also it might be a little confusing because desktop is also going to use webDatabase, right?

web/database/worker/web-db-worker.js
6 ↗(On Diff #23505)

Should we check if this array has at least one element?

  • update type
  • update worker path
  • add check
keyserver/src/responders/webworker-responders.js
16 ↗(On Diff #23505)

right, good call, updating to simple database

web/database/worker/web-db-worker.js
6 ↗(On Diff #23505)

I think it's impossible for this callback to be called when there is no connection (no MessagePort) and there are no checks everywhere in the docs examples.
But on the other hand there is no assurance in API that this array will always have element so adding a check

web/types/worker-types.js
3–5 ↗(On Diff #23505)

I think it works because of problems with how SharedWorkers are typed in the flow, but you're right - this shouldn't work. I'll make this type inexact as you suggested

tomek added 1 blocking reviewer(s): michal.
tomek added inline comments.
keyserver/src/responders/webworker-responders.js
14

Why do we have to specify this for notif worker and not for database one?

keyserver/src/responders/webworker-responders.js
14

The Service-Worker-Allowed header narrows the scope (more detailed scope is specified on registering) from which the Service Worker can intercept requests (where it's a proxy).

Shared Worker is not capable of intercepting calls at all, it just exists for the entire origin so there is no need for that,

For notifs we use Service Worker, for database we use Shared Worker.

This revision is now accepted and ready to land.Mar 15 2023, 10:52 AM
This revision was automatically updated to reflect the committed changes.