Skip to content

fix(websocket): add Uint8Array to WSMessageReceive #4036

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

Closed
wants to merge 1 commit into from

Conversation

yusukebe
Copy link
Member

@yusukebe yusukebe commented Apr 1, 2025

This will fix the issue pointed out in honojs/middleware#1077 (review)

In @hono/node-ws, the data in the link can be Buffer:

https://github.com/honojs/middleware/blob/b18f24379bb4a1350d0b1da0cb109787dddc5193/packages/node-ws/src/index.ts#L116

But currently, WSMessageReceive can not be Buffer and the type error throws:

CleanShot 2025-04-01 at 17 51 07@2x

So I added the Uint8Array, which is the superclass of Buffer to WSMessageReceive.

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

Copy link

github-actions bot commented Apr 1, 2025

Bundle size check

main (882e75a) #4036 (bf97e7c) +/-
Bundle Size (B) 18,976B 18,976B 0B
Bundle Size (KB) 18.53K 18.53K 0K

Compiler Diagnostics

main (882e75a) #4036 (bf97e7c) +/-
Files 261 261 0
Lines 116,442 116,442 0
Identifiers 114,439 114,439 0
Symbols 303,843 303,843 0
Types 214,833 214,833 0
Instantiations 3,091,594 3,091,594 0
Memory used 447,955K 447,914K -41K
I/O read 0.02s 0.02s 0s
I/O write 0s 0s 0s
Parse time 0.65s 0.88s 0.23s
Bind time 0.29s 0.33s 0.04s
Check time 5.87s 5.94s 0.07s
Emit time 0s 0s 0s
Total time 6.81s 7.15s 0.34s

Reported by octocov

Copy link

codecov bot commented Apr 1, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.32%. Comparing base (882e75a) to head (07607eb).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4036   +/-   ##
=======================================
  Coverage   91.32%   91.32%           
=======================================
  Files         168      168           
  Lines       10684    10684           
  Branches     3032     3062   +30     
=======================================
  Hits         9757     9757           
  Misses        926      926           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yusukebe
Copy link
Member Author

yusukebe commented Apr 1, 2025

Hey @nakasyou

Can you review this?

@nakasyou
Copy link
Contributor

nakasyou commented Apr 1, 2025

I think it is not Web Standard. And existing code may break. For example:

upgradeWebSocket({
  async onMessage(evt) {
    let text: string
    if (evt.data instanceof ArrayBuffer) {
      text = decoder.decode(new Uint8Array(evt.data))
    } else {
      // process as blob
      text = await evt.data.text() // TypeScript will throw an error
    }
  }
})

So, an idea, how about using this approach?
https://github.com/nakasyou/hono-middleware/blob/53dfda6828ba560183a515ba641fd826f515d4c4/packages/node-ws/src/index.ts#L122

@yusukebe
Copy link
Member Author

yusukebe commented Apr 1, 2025

@nakasyou

So, an idea, how about using this approach?

Great! But the test will fail:

CleanShot 2025-04-01 at 18 23 36@2x

But I think receivedMessage should not be Buffer. So it's good to rewrite the test? If all is okay, can you create a PR for @hono/node-ws?

@nakasyou
Copy link
Contributor

nakasyou commented Apr 1, 2025

@yusukebe

So it's good to rewrite the test?

Yes, I agree with you.

If all is okay, can you create a PR for @hono/node-ws?

Of course! Check honojs/middleware#1094.

@yusukebe
Copy link
Member Author

yusukebe commented Apr 1, 2025

We follow the honojs/middleware#1094 instead of this PR. Closing.

@yusukebe yusukebe closed this Apr 1, 2025
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.

2 participants