Page MenuHomePhabricator

[web-db] implement basic app-worker connection types
ClosedPublic

Authored by kamil on Mar 7 2023, 9:00 AM.
Tags
None
Referenced Files
F1664955: D6991.diff
Thu, Apr 25, 7:25 PM
Unknown Object (File)
Wed, Apr 3, 4:04 PM
Unknown Object (File)
Wed, Apr 3, 8:28 AM
Unknown Object (File)
Wed, Apr 3, 8:28 AM
Unknown Object (File)
Wed, Apr 3, 8:27 AM
Unknown Object (File)
Wed, Apr 3, 8:19 AM
Unknown Object (File)
Sun, Mar 31, 4:13 PM
Unknown Object (File)
Feb 22 2024, 11:49 PM
Subscribers

Details

Summary

Adding basic types and concept of sharing information between worker and app. Current type (PING/PONG) will be used probably only for development, will add more concrete ones in next diffs.

Depends on D6990

Test Plan

Run this code:

const webDbWorker = new SharedWorker('/worker/webDatabase');
webDbWorker.port.onmessage = function (e) {
  console.log('message from worker: ', e.data);
};
const message: WorkerRequestMessage = {
  type: workerRequestMessageTypes.PING,
  text: 'hi from app',
};
webDbWorker.port.postMessage(message);

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

Check error flow.

Diff Detail

Repository
rCOMM Comm
Branch
db-on-web-2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
web/database/worker/web-db-worker.js
40

That's a problematic line but unfortunately messageEvent.data is typed in flow as mixed and there isn't a better way to cast this to our exact type.

We can not override MessageEvent because even if we override all types there is a place where there will be a need to cast via any (there always will be a difference between what we have and flow's SharedWorker definition).

I think this is the safest place because we will receive data from the other side of the pipe here

kamil published this revision for review.Mar 7 2023, 10:06 AM
web/database/worker/web-db-worker.js
40

This is fine, and I think you're doing it well

web/database/worker/web-db-worker.js
40

Thanks for confirmation

tomek added inline comments.
web/database/worker/web-db-worker.js
24

At this point it looks like we can make this mandatory, but maybe that will change in the future

46–48

I think it might be a good idea to include testing this error scenario in the test plan

This revision is now accepted and ready to land.Mar 15 2023, 3:40 AM
web/database/worker/web-db-worker.js
24

Yes, it will, there will be messages (like INIT) for which resolving/rejecting promises will be the only information. We could avoid sending additional response data between app<->worker with success status when promise could do the same with fewer data transmitted.

I know it's not logically consistent for this diff but the following diff in the stack (D6993) shows why this should not be mandatory.

46–48

Oops, I test that but didn't include it in the test plan, fixing