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

doc: clarified stream.Transform callback argument logic #48653

Closed
wants to merge 1 commit into from
Closed

doc: clarified stream.Transform callback argument logic #48653

wants to merge 1 commit into from

Conversation

rafasofizada
Copy link
Contributor

Clarified that stream.Transform callback second argument is passed only if the first argument is null, i.e. no error occured processing the chunk.

From the way the documentation is written right now, I expected the second argument to be forwarded to transform.push(), even if the first argument is an Error. I was implementing a truncation stream, and passed both an Error ("stream exceeded maximum size"; could be optionally caught and ignored), and the last (truncated) chunk. Only after digging through the relevant implementaton I could confirm for sure that the second argument is entirely ignored if the first one is not null.

Please tell me if you want to adjust the wording somehow, or whether I should add an alternative example to the one following the line "In other words, the following are equivalent:". Hopefully you'll find this useful.

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Jul 5, 2023
@@ -4517,7 +4517,8 @@ The `callback` function must be called only when the current chunk is completely
consumed. The first argument passed to the `callback` must be an `Error` object
if an error occurred while processing the input or `null` otherwise. If a second
argument is passed to the `callback`, it will be forwarded on to the
`transform.push()` method. In other words, the following are equivalent:
`transform.push()` method, but only if the first argument is `null`. In other
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`transform.push()` method, but only if the first argument is `null`. In other
`transform.push()` method, but only if the first argument is falsy. In other

@rafasofizada
Copy link
Contributor Author

@lpinca thanks for the approval! Could you please advice how would I go about editing the commit message to pass the lint-commit-message check?

@lpinca
Copy link
Member

lpinca commented Jul 6, 2023

Wrap the commit message body to 72 chars per line. Also use an imperative verb for commit message title (clarified -> clarify).

doc: clarify transform._transform() callback argument logic

Clarify that `transform._transform()` callback second argument is
used only if the first argument is `null`, i.e. no error occured
processing the chunk.

Clarify that streamTransform callback second argument is passed
only if the first argument is `null`, i.e. no error occured
processing the chunk.
@rafasofizada
Copy link
Contributor Author

@lpinca I tried to amend the commit message and force-push it, but apparently messed something up. Opened a new PR with the proposed changes and fixed commit message: #48680

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants