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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 66 commits into from
Nov 28, 2023
Merged

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

merged 66 commits into from
Nov 28, 2023

Conversation

manekinekko
Copy link
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
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
Collaborator Author

manekinekko commented Nov 10, 2023 •
edited
Loading

Thanks!

@badeend
Copy link

badeend commented Nov 11, 2023

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

@manekinekko
Copy link
Collaborator Author

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
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
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.

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

// TODO: move to a common
Copy link
Collaborator

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
Collaborator Author

Choose a reason for hiding this comment

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

Correct

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

this[symbolSocketState].lastErrorState = null;

const err = this.#socket.listen(this[symbolSocketState].backlogSize);
Copy link
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
Collaborator Author

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
Collaborator Author

@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
5 checks passed
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