Bring back old async support PR

[damusix]

Rebased with hapijs/vision master. References #177

[wswoodruff]

Suggested change

[Marsup]

Suggested change

	return await this._render(compiled, context, request);
	return this._render(compiled, context, request);

[damusix]

lol ahhhhhh! #205 (comment)

[Marsup]

Oops.

[Marsup]

Actually this one is still valid, return await is an anti-pattern.

[devinivy]

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.

[Marsup]

@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?

[devinivy]

@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.

[wswoodruff]

I'd say in visions case, I favor the stacktrace

[damusix]

@Marsup @wswoodruff done!

[wswoodruff]

Looks good! — thank you for your work on this!