Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream objectMode implementation callbacks #39

Open
shirouto opened this issue Aug 19, 2020 · 4 comments
Open

Stream objectMode implementation callbacks #39

shirouto opened this issue Aug 19, 2020 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@shirouto
Copy link

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.

@austindd
Copy link
Collaborator

austindd commented Aug 20, 2020

@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.

@austindd austindd self-assigned this Aug 21, 2020
@austindd austindd added the bug Something isn't working label Aug 21, 2020
@austindd
Copy link
Collaborator

@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: #40

@shirouto
Copy link
Author

@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.

@shirouto
Copy link
Author

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants