Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(worker-api/impl): pass stream to parent #324

Merged
merged 24 commits into from
Jan 1, 2024

Conversation

Aslemammad
Copy link
Contributor

Resolves #308

We implemented our messaging method, but streams are already transferrable between the worker and the main thread. The only issue is that the types are not supporting them although the implementation does support them, that's why I used as unknown as TransferListItem

Copy link

vercel bot commented Dec 27, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Jan 1, 2024 11:23pm

Copy link

codesandbox-ci bot commented Dec 27, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 204a34e:

Sandbox Source
Vanilla Typescript Configuration
React Configuration
React TypeScript Configuration

@himself65
Copy link
Contributor

Upstream issue: DefinitelyTyped/DefinitelyTyped#67980

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far!

Can we do the same thing for main -> worker too?

packages/waku/src/lib/handlers/dev-worker-impl.ts Outdated Show resolved Hide resolved
Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this looks great. I found a way to make it much simpler.

import type { Worker as WorkerOrig } from 'node:worker_threads';
import type {
TransferListItem,
Worker as WorkerOrig,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I recently use WorkerType for the naming convention instead of WorkerOrig. What do you think?

packages/waku/src/lib/handlers/dev-worker-api.ts Outdated Show resolved Hide resolved
packages/waku/src/lib/handlers/dev-worker-api.ts Outdated Show resolved Hide resolved
const err =
mesg.err instanceof Error ? mesg.err : new Error(String(mesg.err));
controller.error(err);
mesg.stream.pipeTo(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary.
So, we don't need mesg.type=pipe at all, and attach the stream for mesg.type=render.

const controller = controllerMap.get(mesg.id)!;
controller.close();
} else if (mesg.type === 'err') {

const controller = controllerMap.get(mesg.id)!;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need controllerMap.

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, if my previous comment was unclear.

const handleRender = async (mesg: MessageReq & { type: 'render' }) => {
const handleRender = async (
mesg: MessageReq & { type: 'render' },
stream: ReadableStream,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking we can simply use rr.stream. Any issue with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you mean just use what's coming from the message?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, can we do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try my best!

},
});

mesg.stream?.pipeThrough(bridge);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bridge is almost no-op, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify what a no-op is?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean it doesn't do anything. "no operation".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't it converting array buffer also to Uint8Array?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we no longer use post message for stream, we shouldn't need any conversion.

@Aslemammad
Copy link
Contributor Author

Aslemammad commented Jan 1, 2024 via email

@dai-shi
Copy link
Owner

dai-shi commented Jan 1, 2024

I don't know if it's related with conversion. Never mind.

@@ -50,36 +44,21 @@ const handleRender = async (mesg: MessageReq & { type: 'render' }) => {
searchParams: new URLSearchParams(rr.searchParamsString),
method: rr.method,
context: rr.context,
body: rr.stream,
body: rr.stream!,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we assure that it's not undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I thought about this, and I could not find a solution, it required so many types gymnastics which I then could not get it right. Let me know if you have an idea.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, it's optional. 204a34e should do for now. We can probably accept undefined so that we don't need such syntax.

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Thanks for your work.

@dai-shi dai-shi merged commit 58423ef into dai-shi:main Jan 1, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

refactor idea: worker-api/impl
3 participants