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: use synchronous error validation & validate abort signal option #41777

Merged

Conversation

iMoses
Copy link
Contributor

@iMoses iMoses commented Jan 30, 2022

made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jan 30, 2022
@iMoses iMoses force-pushed the stream-sync-error-validations branch from 49b4925 to 347b307 Compare January 30, 2022 18:41
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Left some comment about the difference between errors in generators/async functions in this API other than that looks good thanks 🙏

made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option
@iMoses iMoses force-pushed the stream-sync-error-validations branch from 347b307 to 2e4f992 Compare January 31, 2022 15:08
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Generally looks good 🙇

Requesting CI: but I suspect it won't pass and there will be a few lint errors but I want to see the tests themselves passing :)

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Comment on lines +192 to +194
if (options?.signal != null) {
validateAbortSignal(options.signal, 'options.signal');
}
Copy link
Member

Choose a reason for hiding this comment

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

This can be simplified to validateAbortSignal(options?.signal, 'options.signal') (also in other places)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know 👍

Comment on lines +124 to +125
assert.rejects(() => Readable.from([]).reduce((x, y) => x + y, 0, 1), /ERR_INVALID_ARG_TYPE/);
assert.rejects(() => Readable.from([]).reduce((x, y) => x + y, 0, { signal: true }), /ERR_INVALID_ARG_TYPE/);
Copy link
Member

Choose a reason for hiding this comment

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

these two should probably have .then(common.mustCall()) (e.g. like you did in test-stream-some-every)

Copy link
Member

@Linkgoron Linkgoron Feb 3, 2022

Choose a reason for hiding this comment

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

You can also add it to the two lines above them (that you didn't add).

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit 06625ff into nodejs:master Feb 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 06625ff

@iMoses
Copy link
Contributor Author

iMoses commented Feb 3, 2022

@Linkgoron would you like me to take care of your notes in a future PR?

VoltrexKeyva pushed a commit to VoltrexKeyva/node that referenced this pull request Feb 3, 2022
made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option

PR-URL: nodejs#41777
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option

PR-URL: #41777
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
@danielleadams

This comment was marked as outdated.

@danielleadams danielleadams added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 28, 2022
@danielleadams danielleadams added backport-blocked-v16.x and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 2, 2022
targos pushed a commit that referenced this pull request Jul 12, 2022
made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option

PR-URL: #41777
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option

PR-URL: #41777
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
made sure top level methods aren't async/generators
so that validation errors could be caught synchronously
also added validation for the abort signal option

PR-URL: nodejs/node#41777
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Nitzan Uziely <linkgoron@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants