- Open Source
Fixing recurrent need
The project I was working on was entierly promised-based (and later on, using
We also use power-assser, a great assertion library which provides very detailed error report and sticks to nodejs’s assert module API.
We wrote plenty of tests checking that tested code was rejecting promises:
If you forget to check the actual message, then you’re not sure that tested code failed for the expected reason.
And here it comes:
The logic in
catch clause is pretty verbose:
- it checks that an
- it checks an expected pattern in the actual error message
- it stops the test.
throw ensure the code has failed for the good reason.
We wrote the above dozens and dozens of time… With synchronous code, we could have wrote:
This is when I decided to propose a change in nodejs’ API.
One does not simply propose a Pull Request
It was Christmas time, I was full of energy and motivation. I downloaded Node’s code on my Windows machine, read how to build, contribute, write code, write tests and issue a PR.
It took me 2 days (mostly because building and running tests on Windows is crzay long).
Then I made a first pull request… …and retrospectively I’m really ashamed of the naiveness of my contribution:
I turned the existing
assert.doesNotThrow() to async to they could support my usecase.
I clearly overlooked the impacts of my change, on some functions intensively used in a lot of NPM modules, and in the core itself. Even the few tests I wrote prouved I was wrong :’(
But thanks the great kidness and patience of the all my reviewers, I didn’t lost faith and retried few days later.
The second pull request (7th of January) was more acceptable: the idea was to add
assert.doesNotReject() to be the promise-based equivalent of the existing functions.
It took me few iterations (11th, 18th of January and 3rd of February) to fully understand my reviewer’s directives, and provides something truely aligned with the existing code base and documentation.
Then the PR was suspended for a month, because of side discussions about removing
assert.doesNotThrow(). Then it would have been a non-sense to add
But finally, on the 11th of March, the PR was landedon the main branch, and will be part of Node 10!
This was a extremelly long, insightful and educating experience.
Until know, I always shared my side projects as OSS, but actually never worked with an OSS community (except on OptiKey). I worked with a lot of different people and teams at work (sometime up to 30 devlopers), but always on site.
I really learnt:
the benefits of mentoring Nodejs codebase is completly different from modern project (React/Angular/Mongo…) codebases. The core contributors truely taught me how it worked and what to do to perform consistent changes and additions.
it takes patience… OSS is often made by volonteers working during their free time, which necessary makes iteration longer. I was working mostly during weekends myself, hence why this simple feature took almost 3 months.
That was really a great experience, which I hope I could reiterate in the future!