uber / tchannel-node

MIT License
203 stars 40 forks source link

Add Node 12 support #357

Closed micburks closed 4 years ago

micburks commented 4 years ago

We're looking to support Node 12 now that it's in LTS. Since tchannel supports such a wide range of Node versions, and also depends on so many n-api dependencies, it seems to be increasingly difficult to support all Node versions (e.g. the farmhash version that supports Node 12, no longer supports 4 and 6, and no longer runs tests in 8).

I'm unaware of the state of tchannel, but locally I was unable to get all tests to pass in any Node version. I've only superficially looked into why tests are failing. If you have any suggestions, I'd be happy to hear them.

Either way, it seems unlikely to support all Node versions at this time. ~What I'd like to propose is maintaining a node-12 branch of tchannel that supports 10/12, where we can publish a non-mainline version.~ After a quick discussion, the ask is to make this breaking change and bump tchannel to version 4, dropping support for Node <8.

This PR

Tested on 8, 10, and 12

I added a lock file because I wasn't sure if tests were failing from some broken dependency falling through the semver ranges.

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

kriskowal commented 4 years ago

I’m open to bumping the major version and truncating the legacy Node.js support testing. Further releases to the current major version can occur on a parallel train.

micburks commented 4 years ago

Closed by https://github.com/uber/tchannel-node/pull/359

Much appreciated!