feat - Shared Workers

[bebraw]

Work in progress

The purpose of this PR is to add support for Shared Workers to threads.js. So far I've done the following:

• I extracted a get-expose.ts helper to allow a separate expose for the shared cases
• I sketched out initial test infrastructure (currently the tests for the new functionality fail)
• I exposed SharedWorker from threads.js

As I wasn't sure which formatting convention is used on the project, it seems my editor applied some default Prettier formatting. I can fix this later after you let me know of your preference.

I don't expect the code to work yet but there's enough to see the direction.

Closes #401.

[bebraw]

@andywer I've made some progress on this one. The main part I'm not understanding has to do with the proxying at spawn. It's a bit unclear what to do there for shared workers.

I noticed shared workers don't have a terminate method, so I added something based on close() for now although the tests I added aren't passing yet.

[andywer]

Thanks for this massive contribution, @bebraw! I will have a more in-depth look tomorrow and will also try to explain a few things about the code (I know, it's pretty complex and hard to read in some places).

Regarding the formatting… I didn't set up a prettier config, but tslint is set up. So as far as I am concerned, if tslint doesn't complain it should be fine.

[bebraw]

Regarding the formatting… I didn't set up a prettier config, but tslint is set up. So as far as I am concerned, if tslint doesn't complain it should be fine.

Yeah, it's possible we might have to revert the prettier formatting changes. I know tslint isn't very opinionated when it comes to formatting.

[andywer]

@bebraw Especially the semicolons really bloat the diff and make it hard to read. Is there any easy way for you to strip the semicolons? Maybe add some preliminary prettier config or so.

[bebraw]

@bebraw Especially the semicolons really bloat the diff and make it hard to read. Is there any easy way for you to strip the semicolons? Maybe add some preliminary prettier config or so.

Sure, let me do that. 👍

[bebraw]

@andywer It should be cleaner.

[andywer]

Yes, but it was only applied to some files, not all of them. Was that expected?

[bebraw]

@andywer Let me check the remaining ones. :)

[bebraw]

I was able to simplify the code somewhat based on your comments. I realized it's possible to reuse the existing expose without a factory. I added a bit of logic to detect the worker type for this. The change also simplifies the API a notch.

Note that the shared worker related tests don't pass at the moment but those might be fixed as we resolve the last major issue of proxying shared workers.

[andywer]

Great stuff!

as we resolve the last major issue of proxying shared workers.

Is there any way I can help?

[bebraw]

Is there any way I can help?

There is one issue:

 ~/threads.js  feat/shared-workers  npm run test:puppeteer:basic                          ✔  system Node

> [email protected] test:puppeteer:basic /threads.js
> puppet-run --plugin=mocha --bundle=./test/workers/:workers/ --serve=./bundle/worker.js:/worker.js ./test/*.chromium*.ts

✔ Bundling done.

  threads in browser
    1) can spawn and terminate a thread
ReferenceError: SharedWorker is not defined
    at Object.isWorkerRuntime (src/worker/implementation.browser.ts:16:31)
    at Object.<anonymous> (src/worker/index.ts:202:97)

For a reason I don't understand yet, SharedWorker isn't available in that file.

---

The second issue had to do with proxy typing but I managed to solve that one so above is what remains. It's possible I missed something in the code flow and maybe more code is needed.

[andywer]

There is one issue:

I have been thinking about it on and off since yesterday, but that's also weird for me… I don't see how, but maybe the worker is somehow run as a page script, not as a worker?

And always helpful: Have you tried running puppet-run with --inspect? It will not run Chrome headless, but open a window, so you can poke around.

[bebraw]

I have been thinking about it on and off since yesterday, but that's also weird for me… I don't see how, but maybe the worker is somehow run as a page script, not as a worker?

And always helpful: Have you tried running puppet-run with --inspect? It will not run Chrome headless, but open a window, so you can poke around.

Thanks, that was a good tip. I found out that the check against SharedWorker was the wrong thing to do. Instead, I changed it to check against self.port since I assume that's available within a worker itself.

Instead of erroring, now the shared worker related tests don't complete. It seems that const helloWorld = await spawn<() => string>(sharedWorker); never finishes so I'll debug that next.

[bebraw]

It seems it times out at const initMessage = await withTimeout(receiveInitMessage(worker), timeout, ... when spawning.

I think it might be related to the way we're handling expose for shared workers. I wonder if something a bit more custom is needed there. Based on https://developer.mozilla.org/en-US/docs/Web/API/SharedWorker, we can implement onconnect.

[bebraw]

It definitely seems that we're missing onconnect. I started sketching it out and there are some debug prints in place for now. If you have time, feel free to mess around with it.

[pubkey]

What is the status of this feature?
Is there something missing or is it only the tests that fail?

[andywer]

@pubkey Great question. Not sure what the exact status is, but it's definitely marked as "work in progress" (as in not yet "ready for review").

[bebraw]

It turns out my client doesn't need the feature anymore (different approach now).

@pubkey If you want to complete the work, that would be great.