search

thriftrw / thriftrw-node

A thrift binary encoding library using bufrw
MIT License
57 stars 25 forks source link

Remove buffers `noAssert` argument #156

Closed BridgeAR closed 4 years ago

BridgeAR commented 6 years ago

Support for the noAssert argument dropped in the upcoming Node.js v.10. This removes the argument to make sure everything works as it should.

Refs: https://github.com/nodejs/node/pull/18395

Please note: not all tests pass with this change. As far as I see it either the failing test cases should be removed (Node.js itself is going to throw) or the validation has to be done twice with a specific error handling in the code here.

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.

BridgeAR commented 6 years ago

Ping

kriskowal commented 6 years ago

I’ve pushed a no-assert branch. Please rebase on master and cherry-pick this change. It adds guards that fix the remaining errors.

kriskowal commented 6 years ago

(Also, if you wish, I can commandeer this and try my best to preserve your authorship.)

BridgeAR commented 6 years ago

@kriskowal thanks a lot! I just cherry-picked the commit and rebased to master.

About the authorship: please feel free to land this how ever you want.

BridgeAR commented 6 years ago

Ping @kriskowal

kriskowal commented 6 years ago

Further work needed to get the tests passing in 0.10.

BridgeAR commented 6 years ago

@kriskowal I am a bit surprised that you still support 0.10. There are no security patches for it anymore and it is long EOL.

kriskowal commented 4 years ago

I’ve subsumed this change in #172 to get tests passing in new and old Node.js. Let’s just assume for the purposes of Github that I’m overly dogmatic about backward compatibility and not actually using Node.js 0.10 in production anywhere for real.