Skip to content

Conversation

@amydevs
Copy link
Contributor

@amydevs amydevs commented Jun 28, 2024

Description

Allows WebSocketClient and WebSocketConnection to work with the browser WebSocket implementation.

image

To note some discoveries:

  • We need to polyfill Buffer
  • We need to polyfill perf_hooks with the browser performance API to work with browsers.

Issues Fixed

Tasks

  • 1. Make WebSocketClient work with browser WebSockets
  • 2. Make WebSocketConnection work with browser WebSockets
  • 3. Write tests to mock a browser WebSocket using ws library.
  • 4. End to end test using browser React bundle.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

@amydevs
Copy link
Contributor Author

amydevs commented Jun 28, 2024

webpack/webpack#13585

How i got perf_hooks working ^

@amydevs amydevs merged commit 1ce31b9 into staging Jun 28, 2024
};

protected handleSocketError = (err: Error) => {
protected handleSocketError = (err: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the cause can either be an Error or an event. It doesn't rlly matter what this value is, as it's just set as the cause of the thrown error

Copy link
Contributor

Choose a reason for hiding this comment

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

Using any type tends to be pretty hacky. Generally if the type isn't well defined and it doesn't really matter then unknown can be used instead of any.

While any can be anything and assigned to anything. unknown could be anything but can't be assigned to anything else. So it's safer to use. https://stackoverflow.com/questions/51439843/unknown-vs-any

@CMCDragonkai
Copy link
Member

webpack/webpack#13585

How i got perf_hooks working ^

That's a downstream thing right? Not something this repo would do. Because it produces an npm package.

However you can update the readme to explain too.

@amydevs
Copy link
Contributor Author

amydevs commented Jun 29, 2024

@CMCDragonkai yes, it would be something that downstream would have to do. Although, we can automatically apply the polyfill in js-timer directly if we want to.

@CMCDragonkai
Copy link
Member

@CMCDragonkai yes, it would be something that downstream would have to do. Although, we can automatically apply the polyfill in js-timer directly if we want to.

No we shouldn't preemptively polyfill. So readme it is.

Did you read my review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Isomorphic js-ws

4 participants