Skip to content

feat: add impl for node sockets (wip)#214

Merged
guybedford merged 66 commits into
bytecodealliance:mainfrom
manekinekko:feat/nodejs-socket
Nov 28, 2023
Merged

feat: add impl for node sockets (wip)#214
guybedford merged 66 commits into
bytecodealliance:mainfrom
manekinekko:feat/nodejs-socket

Conversation

@manekinekko
Copy link
Copy Markdown
Collaborator

@manekinekko manekinekko commented Oct 17, 2023 •
edited
Loading

A work in progress PR that adds support for node sockets.

Closes #154 (check issue for progress)

@manekinekko manekinekko self-assigned this Oct 17, 2023
@manekinekko manekinekko added the enhancement New feature or request label Oct 18, 2023
@manekinekko manekinekko added this to the Preview 2 milestone Oct 18, 2023
@manekinekko manekinekko force-pushed the feat/nodejs-socket branch 5 times, most recently from 12ffdd8 to 8c40715 Compare October 30, 2023 17:41
@manekinekko manekinekko force-pushed the feat/nodejs-socket branch 2 times, most recently from 49c868b to 06f068e Compare November 7, 2023 17:02
@manekinekko manekinekko added wasi-preview2 This issue is about the Wasi Preview 2 API target-nodejs This issue is about the Node.js target labels Nov 9, 2023
@yoshuawuyts
Copy link
Copy Markdown
Member

yoshuawuyts commented Nov 10, 2023 •
edited
Loading

@manekinekko I've updated #154 to reflect the latest WIT definitions. You may want to check your list against that. I believe this should result in slightly less work.

The main difference is that the symbols are now namespaced. Plus the manual drop- methods have been replaced, by what I presume is the built-in drop lifecycle method. This will still need to be exposed through Symbol.dispose.

@manekinekko
Copy link
Copy Markdown
Collaborator Author

manekinekko commented Nov 10, 2023 •
edited
Loading

Thanks!

@badeend
Copy link
Copy Markdown
Member

badeend commented Nov 11, 2023

The wasi-socket WITs have just been updated. Hopefully for the last time before Preview2

@manekinekko
Copy link
Copy Markdown
Collaborator Author

manekinekko commented Nov 15, 2023

Thank you @badeend for the heads up :)

@manekinekko manekinekko force-pushed the feat/nodejs-socket branch 2 times, most recently from 1e9d88e to bde9d63 Compare November 17, 2023 09:48
@manekinekko manekinekko marked this pull request as ready for review November 19, 2023 13:06
@manekinekko manekinekko mentioned this pull request Nov 24, 2023
Closed
@manekinekko manekinekko changed the title WIP: feat: add impl for node sockets feat: add impl for node sockets (wip) Nov 27, 2023
@manekinekko
Copy link
Copy Markdown
Collaborator Author

manekinekko commented Nov 27, 2023 •
edited
Loading

This PR adds ~80% of the wasi-socket implementation for node.js. As suggested, we can merge this PR as-is and work on the remaining impl + tests (most of then are tcp/udp stream-related).

guybedford
guybedford approved these changes Nov 27, 2023
View reviewed changes
Copy link
Copy Markdown
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Per discussion today I'm happy to merge this as soon as the tests are passing and work through follow-up PRs from there.

Comment thread packages/preview2-shim/lib/nodejs/sockets/tcp-socket-impl.js
serializeIpAddress,
} from "./socket-common.js";

// TODO: move to a common
Copy link
Copy Markdown
Collaborator

@guybedford guybedford Nov 27, 2023

Choose a reason for hiding this comment

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

Are you referring to moving these to the sockets-common.js file here?

Copy link
Copy Markdown
Collaborator Author

@manekinekko manekinekko Nov 28, 2023

Choose a reason for hiding this comment

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

Correct

Comment thread packages/preview2-shim/lib/nodejs/sockets/tcp-socket-impl.js

this[symbolSocketState].lastErrorState = null;

const err = this.#socket.listen(this[symbolSocketState].backlogSize);
Copy link
Copy Markdown
Collaborator

@guybedford guybedford Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It would be good to move to a model where we have the completion callback here.

Copy link
Copy Markdown
Collaborator Author

@manekinekko manekinekko Nov 28, 2023

Choose a reason for hiding this comment

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

Will investigate

@manekinekko manekinekko mentioned this pull request Nov 28, 2023
Closed
@manekinekko
Copy link
Copy Markdown
Collaborator Author

manekinekko commented Nov 28, 2023

@guybedford unit tests are passing. PR should be ready for merge. Will send other PR to migrate to node:net and node:dgram.

@guybedford guybedford merged commit 64475eb into bytecodealliance:main Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybedford guybedford guybedford approved these changes

Assignees

@manekinekko manekinekko

Labels

enhancement New feature or request target-nodejs This issue is about the Node.js target wasi-preview2 This issue is about the Wasi Preview 2 API

Projects

None yet

Milestone

Node.js Preview2

Development

Successfully merging this pull request may close these issues.

Jco transpile: implementation of wasi-sockets for Node.js

4 participants

@manekinekko @yoshuawuyts @badeend @guybedford