reason-nodejs
reason-nodejs copied to clipboard
sikanhe
Stream objectMode implementation callbacks
The ~write callback for Stream.Writable.makeOptionsObjMode has the wrong number of parameters. Even though encoding is ignored for objectMode, it is still required to be provided or else the callback parameter does not make it through.
From what I can tell, this issue extends to most callbacks for objectMode.
@shirouto, thanks for pointing this out. Our lack of a robust test suite is showing here.
I think I understand the cause, which I'll explain just for posterity's sake... It looks like Node does some "argument juggling" inside the public-facing write method to simulate function polymorphism (we already knew this), and then internally calls the user-defined _write method with the full function signature (we overlooked this fact).
I will soon push the new version of this library and include a fix for this. Hopefully I can get that done today. I'll let you know when the PR is out.
@shirouto I think it should be patched with this latest PR. I added a regression test as well. Feel free to clone it and test it out for yourself -- just keep in mind that the Stream type signatures have changed a bit.
PR: https://github.com/sikanhe/reason-nodejs/pull/40
@austindd The new Stream binding seems to work for me. But could you please rebase that branch? It is a few commits behind the master and some of my tests rely on Fs. If you can do that, I will give a more thorough poking.
By the way, I appreciate the increased ergonomics of the new Stream types.
Thinking a bit more about the new write/callback type. They return the write_done abstract type, but that effectively restricts the stream implementation to only synchronous APIs. The Node API does not place such restriction AFAIK:
The callback function must be called synchronously inside of writable._write() or asynchronously (i.e. different tick) to signal either that the write completed successfully or failed with an error.
Am I missing something here? Is there a strong reason for this restriction?