Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this super implementation dependent? I don't think we are testing or supporting what
this
actually refers to. Is there any advantage of this overconst promiseIds = new Set();
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don’t want to guarantee
this
being set to the object passed in (which I think we should guarantee, since it’s the most obvious thing), we should setthis
toundefined
or so. Otherwise other people are going to come to rely on it as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was confusing to me because I expected
this
to refer to the options object. That is howthis
works in methods. However, because we reassign the values internally thethis
object becomes something else.To me, this would be the "obviouse thing":
However, that doesn't work.
Anyway, I'm fine with either adding tests and documentation for this or having no context. I suppose I would prefer the latter, but it is not a big deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having the context be the object passed into
createHooks
would bring symmetry withProxy
handlers, but even in the spec community that was a controversial choice (difficulty to extend the range of handlers w/o breaking web compatibility).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyGu Yeah, I feel doing that brings in much more complexity than it removes.
const promiseIds = new Set();
is such a simple solution.