feat(node): worker_threads by Mesteery · Pull Request #1151 · denoland/std

Mesteery · 2021-08-22T19:11:35Z

No description provided.

Mesteery · 2021-08-22T19:15:30Z

CI errors are related to denoland/deno#9696.

bartlomieju

This looks good @Mesteery, but please some more tests that actually execute some code in workers.

Mesteery · 2021-08-23T16:10:37Z

Yes of course, the more complete tests are already added locally.
The problem is that it's not completely ready, since the implementation of events differs from Node.js (and this causes some problems with worker_threads). So I will rewrite it.

Mesteery

This is almost ready except for...

Mesteery · 2021-08-26T22:40:45Z

@@ -0,0 +1,217 @@
+/// <reference lib="webworker"/>


The tests fail systematically because of the typing errors involved in this reference, so the solution to make it work is --no-check. Do you know another way?

@kitsonk could you please advice on this situation?

Does /// <reference lib="deno.worker" /> fix it?

Apparently not: https://github.com/denoland/deno_std/pull/1151/checks?check_run_id=3857620647

bartlomieju · 2021-10-10T13:07:23Z

Ooops, looks like I borked the merge.

Mesteery · 2021-10-10T13:08:48Z

no worries, it's fixed

bartlomieju · 2021-10-11T21:55:26Z

@Mesteery I discussed this offline with @kitsonk. We believe the way forward right now is to use declare global { } inside this module and not depend on triple-slash reference. There's a bunch of errors coming from usages of globalThis.<property> that could be resolved this way. There are also a few types that are completely missing and TS is complaining about them being any by default.

~~There's also this definition:~~

export const isMainThread = typeof DedicatedWorkerGlobalScope === "undefined" ||
  self instanceof DedicatedWorkerGlobalScope === false;

~~AFAIK DedicatedWorkerGlobalScope is just a type and cannot be used as a value. I suggest to check for existence of window instead (it is only defined in "main" worker in Deno).~~

Would be happy to land it for tomorrow's release if you manage to fix those errors.

lucacasonato · 2021-10-11T21:58:46Z

AFAIK DedicatedWorkerGlobalScope is just a type and cannot be used as a value. I suggest to check for existence of window instead (it is only defined in "main" worker in Deno).

It's an exposed interface too: https://html.spec.whatwg.org/multipage/workers.html#dedicatedworkerglobalscope

bartlomieju · 2021-10-11T22:01:47Z

AFAIK DedicatedWorkerGlobalScope is just a type and cannot be used as a value. I suggest to check for existence of window instead (it is only defined in "main" worker in Deno).

It's an exposed interface too: https://html.spec.whatwg.org/multipage/workers.html#dedicatedworkerglobalscope

Ooops, silly me, I checked it in Chrome's console without spawning a worker 🤦‍♂️

CLAassistant · 2021-10-18T01:26:13Z

All committers have signed the CLA.

kitsonk · 2021-10-18T01:29:26Z

I've amended the files so they type check now.

Side note, I know we went for declare class in our type definitions, but only things declared as var are available on globalThis, which means you've got to jump through some ugly hoops to get the type checking to work.

bartlomieju · 2021-10-19T04:30:05Z


-        emitter.once(name, eventListener);
-        return;
+        emitter.once("error", errorListener);


This line is now failing "Can add once event listener to EventTarget via standalone function" test

Okay, i will fix this.

I disabled this test for now and opened #1455

guybedford · 2022-01-29T15:30:26Z

It's a shame this one got derailed, the PR seemed to be very close!

bartlomieju · 2022-01-29T15:55:50Z

@guybedford this PR is more or less complete, however there's still problem with TS types. I wonder if we should maybe skip typings completely for the time being and just land it.

kt3k · 2022-02-01T10:03:29Z

I think I managed to fix type errors. PTAL @bartlomieju @kitsonk

bartlomieju · 2022-02-01T10:43:28Z

@kt3k this seems fine to me, but I will let @kitsonk take a look before landing.

bartlomieju · 2022-02-04T11:19:59Z

This is a blocker for parcel, so I'm going to land this PR tonight.

littledivy

require and compat APIs are not available to workers. Can we please add that? EDIT: Needs support in CLI

Mesteery · 2022-02-06T11:55:38Z

require and compat APIs are not available to workers. Can we please add that? EDIT: Needs support in CLI

Sorry but I am not familiar with Deno. What do you mean by that?

bartlomieju · 2022-02-06T17:57:39Z

require and compat APIs are not available to workers. Can we please add that? EDIT: Needs support in CLI

Sorry but I am not familiar with Deno. What do you mean by that?

This is something I will work on, currently worker API doesn't respect "compat" mode and doesn't add required globals.

bartlomieju · 2022-03-08T01:39:49Z

After a lot back and forth I found the root problem of the implementation of this PR. Currently tests fails with "Module execution is still pending but there are no pending ops..." suggesting that there's a promise that never resolves. I found that this is caused by parentHandle (which is somewhat an alias of self) is not actually an EventEmitter instance and so events are not properly dispatched. I'm not sure how to solve this problem, if I understood correctly we would have to make self (in the worker context) an instance of EventEmitter but I'm unsure how this could be achieved. Any ideas are welcome.

Alternatively we would have apply a lot of code from EventEmitter manually to copy the behavior (though it still might not be enough due to instanceof checks)

bartlomieju

LGTM, thanks for finishing this @kt3k. And thank you to @Mesteery sorry it took so long to land this!

littledivy

kt3k

@Mesteery Thank you for your initial work!

Mesteery marked this pull request as draft August 23, 2021 00:07

bartlomieju requested changes Aug 23, 2021

View reviewed changes

Mesteery force-pushed the worker_threads branch from e6cb287 to 0612616 Compare August 26, 2021 22:36

Mesteery commented Aug 26, 2021

View reviewed changes

Mesteery requested a review from bartlomieju August 26, 2021 22:41

Mesteery marked this pull request as ready for review August 27, 2021 11:27

Mesteery force-pushed the worker_threads branch from 0612616 to 299b473 Compare August 27, 2021 12:08

Mesteery marked this pull request as draft September 7, 2021 04:04

bartlomieju mentioned this pull request Oct 16, 2021

feat(compat): Node CJS and ESM resolvers denoland/deno#12424

Merged

7 tasks

bartlomieju mentioned this pull request Oct 18, 2021

fix: fix node test setup script #1422

Merged

bartlomieju reviewed Oct 19, 2021

View reviewed changes

bartlomieju marked this pull request as ready for review October 24, 2021 07:44

v1rtl mentioned this pull request Oct 31, 2021

Failed to import - prisma esm-dev/esm.sh#174

Closed

This was referenced Nov 23, 2021

Improve Node Compat Mode denoland/deno#12577

Closed

Cannot find name DedicatedWorkerGlobalScope denoland/deno#12922

Closed

feat(node): worker_threads

0ba2ab4

Mesteery force-pushed the worker_threads branch from 6deed69 to 0ba2ab4 Compare January 29, 2022 18:51

Mesteery requested a review from kt3k as a code owner January 29, 2022 18:51

fix: fix type errors

7bc7107

Merge branch 'main' into worker_threads

9bdda11

ry assigned kt3k Feb 2, 2022

littledivy mentioned this pull request Feb 4, 2022

Goal: Make parcel work! littledivy/napi-rusty-v8#12

Open

10 tasks

Merge branch 'main' into worker_threads

a7aa1a4

littledivy requested changes Feb 4, 2022

View reviewed changes

piscisaureus assigned bartlomieju and unassigned kt3k Feb 8, 2022

fix module_all.ts

7c4148e

bartlomieju reviewed Feb 11, 2022

View reviewed changes

Comment thread node/worker_threads.ts

bartlomieju added 3 commits February 11, 2022 15:25

don't use promise for initial message

b1dd08b

fmt and lint

66cb029

remove console log

b3b03a0

Merge branch 'main' into worker_threads

b3b5d23

kt3k self-assigned this Mar 22, 2022

fix: await initial message in worker

55430d7

bartlomieju approved these changes Mar 22, 2022

View reviewed changes

littledivy approved these changes Mar 22, 2022

View reviewed changes

kt3k approved these changes Mar 22, 2022

View reviewed changes

kt3k merged commit 13d9e91 into denoland:main Mar 22, 2022

Mesteery deleted the worker_threads branch March 23, 2022 13:38

Conversation

Mesteery commented Aug 22, 2021

Uh oh!

Mesteery commented Aug 22, 2021

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

Mesteery commented Aug 23, 2021

Uh oh!

Mesteery left a comment

Choose a reason for hiding this comment

Uh oh!

Mesteery Aug 26, 2021

Choose a reason for hiding this comment

Uh oh!

bartlomieju Sep 6, 2021

Choose a reason for hiding this comment

Uh oh!

kitsonk Oct 10, 2021

Choose a reason for hiding this comment

Uh oh!

Mesteery Oct 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bartlomieju commented Oct 10, 2021

Uh oh!

Mesteery commented Oct 10, 2021

Uh oh!

bartlomieju commented Oct 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucacasonato commented Oct 11, 2021

Uh oh!

bartlomieju commented Oct 11, 2021

Uh oh!

CLAassistant commented Oct 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitsonk commented Oct 18, 2021

Uh oh!

bartlomieju Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

Mesteery Oct 19, 2021

Choose a reason for hiding this comment

Uh oh!

bartlomieju Oct 24, 2021

Choose a reason for hiding this comment

Uh oh!

guybedford commented Jan 29, 2022

Uh oh!

bartlomieju commented Jan 29, 2022

Uh oh!

kt3k commented Feb 1, 2022

Uh oh!

bartlomieju commented Feb 1, 2022

Uh oh!

bartlomieju commented Feb 4, 2022

Uh oh!

littledivy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mesteery commented Feb 6, 2022

Uh oh!

bartlomieju commented Feb 6, 2022

Uh oh!

Uh oh!

bartlomieju commented Mar 8, 2022

Uh oh!

bartlomieju left a comment

Choose a reason for hiding this comment

Uh oh!

littledivy left a comment

Choose a reason for hiding this comment

Uh oh!

kt3k left a comment

Choose a reason for hiding this comment

bartlomieju commented Oct 11, 2021 •

edited

Loading

CLAassistant commented Oct 18, 2021 •

edited

Loading

littledivy left a comment •

edited

Loading