I've been using the boardgame.io framework to create a multiplayer game as of late. It's been going very well, but I did trip over a few quirks of the library. One in particular, I was able to chase down and submit a patch to tweak, which was graciously accepted by the framework maintainers! I thought I'd record just a bit of how that went.
The issue
The library includes a lobby management system that lets the game code host multiple simultaneous games on the same server. There is a convenient Lobby React component that exposes this feature to the client and lets a player choose their name and create / join games. One issue with the component is that it polls the server every two seconds to get the list of games, and this polling continues even when the player is playing a game. It's unnecessary traffic; anecdotally, having the UI open for a half hour consumed about 1MB of data.
I set about adjusting this. After forking the project and downloading a copy of the source from GitHub, I let it build and ran the unit tests to confirm I had a good development environment. One convenient thing about Node.js projects is that they're very consistent in this regard; npm
is a fairly mature package management solution, and a well-constructed Node package describes both how to build a package for production and how to build an environment to develop it, so you generally have what you need to do testing after one npm ci
run. The developers have also helpfully included contribution guidelines, which make it much easier to know how to develop changes to the framework.
The obvious place to start was the component itself, and after a bit of fishing I found the React component definition, containing functions _startRefreshInterval
and _clearRefreshInterval
. Sure enough, these functions got called at component mount and unmount, but not when the component transitioned from listing games to playing a game (or from choosing a player's name to listing games; it would start polling before the list was displayed). I was able to make some tweaks and build out a unit test, and then I set about manual end-to-end testing. This proved very valuable; there were a couple corner cases in my initial attempt that manual testing caught before I pushed a bad patch up to the maintainers.
To make clear my intention, I filed an issue against the main project to give some context on why I was making a change. Even when I think the change is self-explanatory, I find filing an issue to be a good step when proposing a change to someone's GitHub project; it makes it clear why the change is happening and serves as a place for others to comment on the problem itself, not just the proposed fix. I like to follow the "What I did / expected / observed" pattern (where appropriate) to make clear why I think something needs to be changed.
As per the contribution guidelines, I pushed my changes to a new branch and created a pull request to propose the change. I expected to go a couple rounds with the maintainers on getting it just right (in particular, I didn't know how they'd feel about what I'd done with the unit tests), but to my surprise they were happy with it and accepted it on the first pass! The change is now in the latest minor revision, so hopefully others will benefit from fewer poll requests while their games are running.
I haven't done many pull requests on other people's GitHub projects, and this was the smoothest I've experienced. I credit part of it to a lot more practice in my day job on the flow of the process. Hopefully, this write-up can serve as a template for others to make changes they hope to see in code they use. Things that really worked in my favor here, I think:
- This project has a well-defined README, community standards, and contribution guidelines. Read all of those.
- Node.js makes working on somoene else's code very, very simple. It's probably my favorite package management system nowadays, and that's really saying something; I'd pretty much written off package management as unsolvable, and
npm
is proving me wrong. - Full unit test coverage never hurts (nor does it hurt that the maintainer put a coverage-check infrastructure in!).
- Detailed and followable end-to-end testing that checked on corner cases increased confidence in the change (and found some bugs). Do this. Think about corner cases.
- If you take the time as a maintainer to make changes easy and are responsive to changes, you give submitters a positive experience and they're likely to come back!
No comments:
Post a Comment