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

Fixup ws usage for newer versions #16720

Merged
merged 3 commits into from
Apr 22, 2022

Conversation

lewing
Copy link
Contributor

@lewing lewing commented Apr 14, 2022

Upgrade to ws 8.5.0. Fixes #5971 #10264 #12813

@lewing lewing marked this pull request as ready for review April 14, 2022 17:55
@lewing
Copy link
Contributor Author

lewing commented Apr 14, 2022

Looks like the tests/sockets/ws can be removed as well. I think the Buffer wrapping can be removed by setting websocket.binaryType='arraybuffer' but I haven't tested. I'm happy to make those changes if desired.

@sbc100
Copy link
Collaborator

sbc100 commented Apr 14, 2022

Ah yes it looks like tests/sockets/ws have been unused since #7536. Do you want to open a separate PR to remove that?

@sbc100
Copy link
Collaborator

sbc100 commented Apr 14, 2022

Ah yes it looks like tests/sockets/ws have been unused since #7536. Do you want to open a separate PR to remove that?

If not, I can do it.

@lewing
Copy link
Contributor Author

lewing commented Apr 14, 2022

Ah yes it looks like tests/sockets/ws have been unused since #7536. Do you want to open a separate PR to remove that?

sure

@lewing
Copy link
Contributor Author

lewing commented Apr 14, 2022

#16732

@sbc100 sbc100 requested a review from juj April 15, 2022 00:43
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm. I'll let @juj give final approval since he wrote this code and knows it better than me.

@lewing
Copy link
Contributor Author

lewing commented Apr 22, 2022

I see this missed 3.1.8? I know it isn't considered urgent but it would be great if it could make it in soon.

@sbc100 sbc100 merged commit 4b662b2 into emscripten-core:main Apr 22, 2022
@lewing lewing deleted the ws-from-this-century branch April 22, 2022 20:52
tlively pushed a commit that referenced this pull request Apr 27, 2022
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.

TypeError: Cannot read property 'binary' of undefined
2 participants