search

thriftrw / thriftrw-node

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

Promote all IDL from ASCII to UTF-8 #177

Closed kriskowal closed 4 years ago

kriskowal commented 4 years ago

This change replaces the default encoding for Thrift IDL files to UTF-8 for parity with ThriftRW-Go. As UTF-8 is a strict superset of ASCII, this is a backward-compatible change. This change also introduces an encoding option to the Thrift constructor and Thrift.load function, and the setting applies through transitive includes.

The change has ramifications for the new asynchronous loader, Thrift.load, which requires a file system implementation that implements readFile(path, encoding, cb) instead of readFile(path, cb), but was not previously released. The Node.js fs module satisfies this contract just as well, but this change would have broken any user that provided a less sophisticated filesystem shim.

Fixes #151

kriskowal commented 4 years ago

cc @KeitaMoromizato for their contributions, for review.

cc @dbousque for the change to the async filesystem API necessary to facilitate alternate encodings.

dbousque commented 4 years ago

If the encoding argument to model.fs.load is required (looks like it is), I guess we should update the documentation in the Non filesystem and asynchronous source loading part.

kriskowal commented 4 years ago

Having slept on this change, I think it might make more sense to just treat all IDL as UTF-8 and not make the encoding configurable. That would avoid any possible confusion between Thrift compilers provided they converge on UTF-8, which is the only sensible direction up from ASCII today, if we have a prayer of having inter-language interoperability.