Skip to content

Conversation

@gribnoysup
Copy link
Collaborator

Another straightforward one: in some cases we were awaiting non-promises, in most cases it wasn't a big deal, but in one particular case it was actually not doing what's expected as the promise was awaited before passing it to allSettled

@gribnoysup gribnoysup requested a review from a team as a code owner November 26, 2025 11:53
@gribnoysup gribnoysup requested a review from ivandevp November 26, 2025 11:53
@gribnoysup gribnoysup added the no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion) label Nov 26, 2025
@github-actions github-actions bot added the fix label Nov 26, 2025
@gribnoysup gribnoysup added the no release notes Fix or feature not for release notes label Nov 26, 2025
Copy link
Collaborator

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

lgtm!

Comment on lines +2221 to +2224
return (
connectionStorage.delete?.({ id: connection.info.id }) ??
Promise.resolve()
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

(not asking for a change here) This is where I feel like an async (keyword) function is more appropriate than substituting a Promise.resolve. Plus return-await is a rule we employed in the driver because when you don't do that you remove functions from async stack traces.

return await connectionStorage.delete?.({ id: connection.info.id });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does read better, let me see if switching to this will scream at me because I'm awaiting something that is maybe non-thenable (also makes sense in global-fixtures code I guess)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix no release notes Fix or feature not for release notes no-title-validation Skips validation of PR titles (conventional commit adherence + JIRA ticket inclusion)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants