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.
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
Bring back old async support PR #205
Bring back old async support PR #205
Changes from 5 commits
a4fb867
3201dee
96594f7
3fa1b92
4e6e7ac
929fb75
9ff55e2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 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.
lol ahhhhhh! #205 (comment)
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.
Oops.
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.
Actually this one is still valid,
return await
is an anti-pattern.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.
As I see it, it's not necessarily an anti-pattern, as there are benefits and tradeoffs of
return await
. The downside is that it's not necessary to the code and you can expect to allocate an extra promise. The upside is that it enables correct async stack traces and is slightly easier to refactor around. My understanding is that the bulk of the performance downsides were largely addressed internally to V8 for node v12, though you still might omit theawait
in hot paths to squeeze out a little optimization if you're willing to sacrifice the line in the async stack trace. I don't have strong feelings in this case, but if there's a precedent set elsewhere in vision I might suggest we follow that.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.
@devinivy I don't see any optimization related to that kind of pattern, it still creates extra unnecessary promises. A quick test doesn't show a stack difference either. Do you have a snippet that would show a difference?
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.
@Marsup interestingly in that example we don't receive an async stack track, possibly because no full ticks complete before the error is thrown. If you add in any amount of waiting before throwing, you'll see the difference: https://runkit.com/devinivy/60914a9d4b20c6001afb3aa3. The optimization I was referencing applies generally to await— it used to cost two promises and three microticks, and now it costs just one promise and one microtick, so this optimization of avoiding an await bears less fruit than it used to when async/await originally landed.
As far as this PR goes, my recommendation would be to stick with whatever convention is used within vision if there is one, and otherwise either way will be just fine (and we can always change it). Perhaps @Marsup, @damusic, or @wswoodruff has a sense for whether the optimization or the stacktraces are more valuable in this case.
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.
I'd say in
vision
s case, I favor the stacktraceThere 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.