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.

Sign up for GitHub

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

Jump to bottom

stream: remove isPromise utility function #35925

Closed
wants to merge 2 commits into from
Closed

stream: remove isPromise utility function #35925

wants to merge 2 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 2, 2020 •
edited
Loading

The function was not checking if the parameter was actually a Promise
instance, but if it has a then method. Removing the utility function
in favor of a clearer typeof check, handling the case when the
thenable throws if then method is accessed more than once.

The difference between a thenable and a Promise may be important when using third party library (the parameter is provided by the user) and/or when Promise.prototype.then value is changed from userland.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

ronag
ronag requested changes Nov 2, 2020
View reviewed changes
Copy link
Member

@ronag ronag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is an improvement? The behavior is the same?

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 2, 2020

Yes the behavior is the same. Another way would be to change the name of the function isPromise -> isThenable.

@benjamingr
Copy link
Member

Actually the current implementation (and the one in this PR) is wrong if someone passes a thenable where then is a getter that throws the second time then is accessed.

It's not a big bug but promise test suites check for it.

The correct implementation would be to do it in "two steps":

  • First do const then = ret?.then
  • Then check if(typeof then === "function")
  • Then do then.call(ret, ...)

See here for an example

ronag added a commit to nodejs/undici that referenced this pull request Nov 2, 2020
@ronag
fix: retry idempotent with original body
d93d222 
Fixes: nodejs/node#35925
@ronag ronag mentioned this pull request Nov 2, 2020
Merged
@aduh95
stream: remove isPromise utility function
f18db47 
The function was not checking if the parameter was actually a Promise
instance, but if it has a `then` method. Removing the utility function
in favor of a clearer `typeof` check, handling the case when the
thenable throws if then method is accessed more than once.
@aduh95 aduh95 requested a review from ronag November 2, 2020 16:21
@aduh95
Copy link
Contributor Author

aduh95 commented Nov 2, 2020

I've implemented the solution suggested by @benjamingr, PTAL.

benjamingr
benjamingr approved these changes Nov 2, 2020
View reviewed changes
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.

Please consider adding a test.

If you prefer not to - I'll add one sometime after this lands :]

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 3, 2020

Test added! It does fail on master and not with this PR, PTAL.

@ronag Have your concerns been addressed?

@aduh95
Copy link
Contributor Author

aduh95 commented Nov 8, 2020

@ronag are you still blocking this or can your objection be dismissed?

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 8, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34228/

mcollina
mcollina approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 9, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2020

Landed in adae822...ad98cf0

@github-actions github-actions bot closed this Nov 9, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 9, 2020
@aduh95 @nodejs-github-bot
stream: remove isPromise utility function
ad98cf0 
The function was not checking if the parameter was actually a Promise
instance, but if it has a `then` method. Removing the utility function
in favor of a clearer `typeof` check, handling the case when the
thenable throws if then method is accessed more than once.

PR-URL: #35925
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@aduh95 aduh95 deleted the stream-remove-isPromise branch November 9, 2020 08:32
danielleadams pushed a commit that referenced this pull request Nov 9, 2020
@aduh95 @danielleadams
stream: remove isPromise utility function
b7aa5e2 
The function was not checking if the parameter was actually a Promise
instance, but if it has a `then` method. Removing the utility function
in favor of a clearer `typeof` check, handling the case when the
thenable throws if then method is accessed more than once.

PR-URL: #35925
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams danielleadams mentioned this pull request Nov 9, 2020
Merged
targos pushed a commit that referenced this pull request May 1, 2021
@aduh95 @targos
stream: remove isPromise utility function
26e9c39 
The function was not checking if the parameter was actually a Promise
instance, but if it has a `then` method. Removing the utility function
in favor of a clearer `typeof` check, handling the case when the
thenable throws if then method is accessed more than once.

PR-URL: #35925
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aduh95 @nodejs-github-bot @benjamingr @mcollina @ronag