This repository has been archived by the owner on Feb 26, 2024. It is now read-only.
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.
@gnidan, What does
waitForOutstandingPromisees()
do here? It seems like anop
.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... waits for outstanding promises so we can
process.exit()
cleanlyThere 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's called with no params, so, isn't
iterable
in the implementation just an empty array. Unless, I'm missing some setup in this scenario :/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.
Yeah, hrm. Something weird must be implicitly happening, because this works and matches the usage in cli.js that @benjamincburns put there.
Ben, can you teach us?
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.
tl;dr: This is being used as expected here.
When
waitForOutstandingPromises
is called with no arguments (as it should be in all non-test use cases), the value ofiterable
is only an empty array when there are no outstanding promises on which to wait.The object destructuring assignment for
target
coupled with the ternary operator probably makes this code horrendously unreadable. Sorry about that. Allow me to break things down a bit...First off, on the line where we initialize
target
:We wind up with
target
being assignedundefined
. This happens becauseoptions
isundefined
, so we fallback to the empty object literal and then attempt to destructure thetarget
property out of that empty object literal. The empty object literal has notarget
property, so the destructuring assignment results withtarget
being initialized with a value ofundefined
.Given that detail, the following statement should be a bit more clear:
The value of
target
isundefined
, so we branch to the "else" statement in the ternary operator, which assigns the return value of_outstandingPromiseMap.keys()
to theiterable
variable. This always assigns that return value, as_outstandingPromiseMap
is a module-scoped variable, and it's initialized asnew Map<Promise<any>, boolean>()
on module load (see code here).If there's any further uncertainty, be sure to check the tests for the
promise-tracker
package as they're fairly comprehensive and they demonstrate and guard its usage nicely.@gnidan @cds-amal @cliffoo please let me know if I can be of further assistance.
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.
Thanks @benjamincburns, my question relates to how
_outstandingPromiseMap
is seeded with values. If nothing is inserted, then how does it know what to track? This PR only calls the tracking code (as a JS module) and doesn't have any code to set/identify/decorate what has to be awaited.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.
There's a README.md, if that helps?
Otherwise the necessary code in question is already decorated.
dashboard-message-bus-client
depends onpromise-tracker
, and its internalsend
method is tracked here.Decorating it with the
@tracked
label is all that's necessary to cause it to be tracked.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.
Also if it helps any, this is the function that does the actual task tracking.
The
tracked
decorator injects this logic by returning a property descriptor that replaces the decorated method implementation with a closure over that impl that proxies calls to it and handles tracking of outstanding promises by wrapping any returned promises in special promise handlers that manage the state of_outstandingPromiseMap
and friends.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.
Thanks @benjamincburns, this is the piece I was missing. It was unclear from the code that it relied on an already coded setup.
There should probably be a comment to explain this in the waiting portion, this PR.