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
Unknown Object (File)
Sat, Dec 21, 11:56 AM
Unknown Object (File)
Mon, Dec 16, 8:17 AM
Unknown Object (File)
Sat, Dec 14, 4:27 PM
Unknown Object (File)
Sat, Dec 14, 4:27 PM
Unknown Object (File)
Sat, Dec 14, 4:27 PM
Unknown Object (File)
Sat, Dec 14, 4:18 PM
Unknown Object (File)
Wed, Dec 4, 5:10 AM
Unknown Object (File)
Mon, Dec 2, 9:18 AM
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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
web/database/worker/web-db-worker.js
40 ↗(On Diff #23509)

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 ↗(On Diff #23509)

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

web/database/worker/web-db-worker.js
40 ↗(On Diff #23509)

Thanks for confirmation

tomek added inline comments.
web/database/worker/web-db-worker.js
24 ↗(On Diff #23509)

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

46–48 ↗(On Diff #23509)

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 ↗(On Diff #23509)

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 ↗(On Diff #23509)

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