Bring back old async support PR #205
Conversation
lib/manager.js
Outdated
| return this._render(compiled, context, request); | ||
|
|
||
| return await this._render(compiled, context, request); | ||
|
|
| const compiled = await this._prepare(filename, options); | ||
| return this._render(compiled, context, request); | ||
|
|
||
| return await this._render(compiled, context, request); |
There was a problem hiding this comment.
| return await this._render(compiled, context, request); | |
| return this._render(compiled, context, request); |
There was a problem hiding this comment.
lol ahhhhhh! #205 (comment)
There was a problem hiding this comment.
Actually this one is still valid, return await is an anti-pattern.
There was a problem hiding this comment.
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 the await 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.
@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.
I'd say in visions case, I favor the stacktrace
Plus some style fixes mentioned in PR convo
|
@Marsup @wswoodruff done! |
Rebased with hapijs/vision master. References #177