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

feat: implement WebSocketStream #3560

Merged
merged 22 commits into from
Sep 26, 2024
Merged

Conversation

KhafraDev
Copy link
Member

@KhafraDev KhafraDev commented Sep 6, 2024

With better-organized internals, this is now much easier and almost completely detached from WebSocket. I could rewrite it to be completely independent of WebSocket, which would allow us to remove it at any time (the only shared internals are a new getURLRecord function and the closing logic).

const stream = new WebSocketStream('https://echo.websocket.org/')
const { readable, writable } = await stream.opened

;(async () => {
  /** @type {ReadableStreamReader} */
  const reader = readable.getReader()

  while (true) {
    const { done, value } = await reader.read()
    if (done) break
    console.log('received message', { value })
  }
})();

setInterval(() => {
  console.log('sending message.')
  /** @type {WritableStreamDefaultWriter} */
  const writer = writable.getWriter()

  writer.write('Hello, world!')

  writer.releaseLock()
}, 5000)

@KhafraDev KhafraDev force-pushed the websocketstream-again branch from 1c931a2 to deae218 Compare September 7, 2024 23:11
@KhafraDev KhafraDev marked this pull request as ready for review September 8, 2024 17:27
@KhafraDev KhafraDev force-pushed the websocketstream-again branch from 58d5072 to 1237174 Compare September 8, 2024 17:29
@KhafraDev
Copy link
Member Author

@nodejs/undici This passes the same subset of WebSocketStream tests that Deno passes. It's missing types and docs now. Looking for code reviews before doing the trivial stuff.

lib/web/websocket/util.js Outdated Show resolved Hide resolved
try {
string = webidl.converters.USVString(chunk)
} catch (e) {
return Promise.reject(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

couldnt we reuse the already created deferred promise?

return promise.reject(e)
?

@Uzlopak Uzlopak changed the title websocketstream (again) feat: implement WebSocketStream Sep 9, 2024
@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 9, 2024

I wanted to test the WebSocketError thing i mentioned above and found this.

const { WebSocketError } = require('./lib/web/websocket/stream/websocketerror');

console.log(Object.keys(new WebSocketError('asd')));

It is not possible to instantiate WebSocketError by requiring the file alone.

image

@KhafraDev
Copy link
Member Author

@tsctx @Uzlopak PTAL.

@KhafraDev
Copy link
Member Author

KhafraDev commented Sep 25, 2024

I wanted to test the WebSocketError thing i mentioned above and found this.

A limitation of cjs - there isn't much we can do about shared webidl declarations. If we moved them to webidl.js, we'd have circular imports, if we move them to another file, we have to import another file. You can const { WebSocketError } = require('.') instead. (once I export it... although I'd be willing to bet it's bug-free)

tsctx
tsctx previously requested changes Sep 25, 2024
return
}
} else if (type === opcodes.BINARY) {
chunk = new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
chunk = new Uint8Array(data.buffer, data.byteOffset, data.byteLength)
chunk = data.byteLength === data.buffer.byteLength ? new Uint8Array(data.buffer, data.byteOffset, data.byteLength) : new Uint8Array(data)

Copy buffer if needed.

@KhafraDev
Copy link
Member Author

please approve so I can land... nits can wait, this is experimental

I want to make websocket PRs but I don't want to manage multiple branches at once. ty

@KhafraDev KhafraDev merged commit cfab33f into nodejs:main Sep 26, 2024
31 of 33 checks passed
@KhafraDev KhafraDev deleted the websocketstream-again branch September 26, 2024 01:50
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
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.

4 participants