Contributing to Nodejs

A real open-source experience

Fixing recurrent need

The project I was working on was entierly promised-based (and later on, using async/await).

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:

it('should handle error while getting config from db', async () => {
  try {
    await config.get()
    throw new Error('should have failed')
  } catch (err) {
    assert(err instanceof Error)
    assert(err.message.includes('db failure'))
  }
})

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:

it('should handle error while getting config from db', async () => {
  try {
    await config.get()
  } catch (err) {
    assert(err instanceof Error)
    assert(err.message.includes('db failure'))
    return
  }
  throw new Error('should have failed')
})

The logic in catch clause is pretty verbose:

The final 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:

it('should handle error while getting config from db', async () => {
  assert.throws(config.get(), err => 
    err instanceof Error && err.message.includes('db failure')
  )
})

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.throws() and 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.rejects() and 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 assert.doesNotReject().

But finally, on the 11th of March, the PR was landedon the main branch, and will be part of Node 10!

Takeways

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:

That was really a great experience, which I hope I could reiterate in the future!