search

uber / tchannel-node

MIT License
203 stars 40 forks source link

Updated dependencies to be compatible with Node 9+. #356

Closed ajbogh closed 5 years ago

ajbogh commented 5 years ago

Tested npm install as well as npm rebuild with Node 10.15.3.

kriskowal commented 5 years ago

Have you also tested with Node.js 0.10?

ajbogh commented 5 years ago

@kriskowal 0.10.0 still produces the farmhash error reported in #355. There are no new errors beyond that, or at least the existing exception prevents new errors from displaying.

ajbogh commented 5 years ago

@kriskowal, I tested with Node 0.10.0 and it works as long as a couple of flags are defined with the right values. The error from #355 stems from the Mac version, and a lack of the utility class for farmhash's header file.

Using flags with npm rebuild (replace rebuild with install if desired):

CXXFLAGS="-mmacosx-version-min=10.9" LDFLAGS="-mmacosx-version-min=10.9" npm rebuild

Farmhash has updated their code to v2.1.0 and set the minimum Node version to >4.5.0, so the error from #355 wouldn't be valid with newer dependency versions. Perhaps it would be better to upgrade dependencies rather than fixing bugs for extremely outdated and non-supported versions of Node.