Skip to content

Upgrading socket.io packages#946

Merged
delucis merged 13 commits into
boardgameio:mainfrom
chasen:socketio-upgrade
Jul 8, 2021
Merged

Upgrading socket.io packages#946
delucis merged 13 commits into
boardgameio:mainfrom
chasen:socketio-upgrade

Conversation

@chasen
Copy link
Copy Markdown
Contributor

@chasen chasen commented May 28, 2021

Socket.io latest version comes with some quality of life improvement that will help with the game development I am working on. So that we dont have to run a separate socket.io server and we could instead just hook into the main one used by the game it would be nice to be able to use these features.

All tests run and pass

Closes #945

Checklist

  • Use a separate branch in your local repo (not main).
  • Test coverage is 100% (or you have a story for why it's ok).

Copy link
Copy Markdown
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thanks for this @chasen, this was on our to-do list. A few comments:

  • We don’t actually import socket.io directly anywhere, this is done by koa-socket-2, so we can remove socket.io as a direct dependency. Actually this seems to cause TypeScript to fail to find socket.io, so I guess we just need to make sure we install the same version as koa-socket-2 uses?

  • koa-socket-2 still uses socket.io v3 (source), so we’d be using a v3 server with a v4 client. According to the socket.io v3 → v4 migration guide:

    a v3 client will be able to reach a v4 server and vice-versa

    So this is probably OK, but maybe worth double checking?

  • As of socket.io v3, type definitions are bundled with the main packages (source), so we should remove the @types/socket.io and @types/socket.io-client packages. This does trigger some TypeScript errors though, which will need resolving.

  • Most of the relevant breaking changes to socket.io (v3, v4) don’t seem to impact us but we do need to make some changes to handle how socket.io >3 handles CORS. If I run our examples (you can do this by running npm start from the boardgame.io repo), all the multiplayer examples (except Tic-Tac-Toe > Multiplayer, which uses the Local multiplayer adapter rather than the socket server) are broken on this branch and print CORS errors to the browser console. Ideally we would provide a default set-up that continues working as currently, or give clear guidance on how to configure CORS and apply that guidance to these examples.


(As you can see, the tests can’t be entirely trusted when it comes to socket communications as in general one side or the other gets mocked in tests, so unfortunately a bit of manual testing is necessary until that’s improved.)

@chasen
Copy link
Copy Markdown
Contributor Author

chasen commented May 31, 2021

Sounds good, ill take a stab at updating these over the next few days

@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jul 7, 2021

@delucis PTAL... I think I addressed all your comments. Important to note a new "origins" param for the Server

@delucis
Copy link
Copy Markdown
Member

delucis commented Jul 7, 2021

Thanks for taking this on @vdfdev! This all looks good and the examples are working as expected. One thought: do you think we should throw an error if the origins option is not set? Or log some kind of warning? It is possible to serve the front-end from the same origin as the game server, but I’m guessing the vast majority don’t (as in all our documentation apart from the Heroku deployment guide). Maybe a warning when starting the server would be helpful to people debugging after updating.

delucis
delucis previously approved these changes Jul 7, 2021
@larry801
Copy link
Copy Markdown
Contributor

larry801 commented Jul 7, 2021

@delucis
Copy link
Copy Markdown
Member

delucis commented Jul 7, 2021

@larry801 The updates on this branch will bump the used engine.io version to 4.1.1 and that CVE lists the vulnerable versions as <4.0.0, so I guess merging this would resolve that?

@delucis delucis requested a review from vdfdev July 7, 2021 23:50
@delucis
Copy link
Copy Markdown
Member

delucis commented Jul 7, 2021

@vdfdev I noticed that the origins can also be regular expressions, so I’ve updated the docs to mention that. (Allows us to cover any localhost ports in the tutorial, which is nice as Parcel serves on 1234 by default and Create React App on 3000.) I’ve also added the logging I suggested to help point people to the docs if they update and encounter errors.

@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jul 7, 2021

Hum, Nice, I didn't know it could be a regex. Maybe we can leave the local host with any port as the default? Thanks for adding the warning!

vdfdev
vdfdev previously approved these changes Jul 7, 2021
@delucis
Copy link
Copy Markdown
Member

delucis commented Jul 8, 2021

Maybe we can leave the local host with any port as the default?

I also considered that, but I guess the socket.io CORS default was chosen with security in mind, and requiring setting it gives us the chance to introduce the concept in the tutorial (instead of encountering it for the first time as yet another thing that breaks when deploying to a proper server).

@vdfdev
Copy link
Copy Markdown
Contributor

vdfdev commented Jul 8, 2021

I agree with this trade-off... sounds good then!

@delucis delucis merged commit dffcb18 into boardgameio:main Jul 8, 2021
@delucis
Copy link
Copy Markdown
Member

delucis commented Jul 8, 2021

Thanks for getting the ball rolling @chasen and for getting things working @vdfdev! This is now released as 0.45.0 🎉

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Upgrading socket.io and koa-socket-2

4 participants