Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bring back old async support PR #205

Merged
merged 7 commits into from
May 23, 2021
Merged

Bring back old async support PR #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a4fb867
async global context
damusix Feb 4, 2019
3201dee
update docs
damusix Feb 4, 2019
96594f7
add support for context that returns promise
damusix Feb 4, 2019
3fa1b92
remove async checks in manager
damusix Mar 8, 2019
4e6e7ac
fix: 🐛 rebase with master and fix tests
Dec 22, 2020
929fb75
style: 💄 spacing and async functions
Apr 14, 2021
9ff55e2
test: 💍 assert render promise
Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion API.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ The Views Manager is configured with [`registration options`](#registration) or
Defaults to empty options `{}`.
- `contentType` - the content type of the engine results. Defaults to `'text/html'`.
- `context` - a global context used with all templates. The global context option can be either
an object or a function that takes the [`request`](https://github.com/hapijs/hapi/blob/master/API.md#request)
an object or a function (can be async or return a promise) that takes the [`request`](https://github.com/hapijs/hapi/blob/master/API.md#request)
as its only argument and returns a context object. The [`request`](https://github.com/hapijs/hapi/blob/master/API.md#request) object is only provided when using
the [view handler](#the-view-handler) or [`h.view()`](#hviewtemplate-context-options). When using
[`server.render()`](#serverrendertemplate-context-options-callback) or
Expand Down
19 changes: 15 additions & 4 deletions lib/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ internals.defaults = {
context: null
};


module.exports = class Manager {

constructor(server, options) {
Expand Down Expand Up @@ -381,13 +380,23 @@ module.exports = class Manager {
});
}

_render(compiled, context, request) {

async _render(compiled, context, request, callback) {

if (this._context) {
let base = typeof this._context === 'function' ? this._context(request) : this._context;
damusix marked this conversation as resolved.
Show resolved Hide resolved

let base = this._context;

if (typeof this._context === 'function') {

base = await base(request);
}

if (context) {

base = Object.assign({}, base); // Shallow cloned
const keys = Object.keys(context);

for (let i = 0; i < keys.length; ++i) {
const key = keys[i];
base[key] = context[key];
Expand Down Expand Up @@ -521,8 +530,10 @@ module.exports = class Manager {

async render(filename, context, options, request) {


damusix marked this conversation as resolved.
Show resolved Hide resolved
const compiled = await this._prepare(filename, options);
return this._render(compiled, context, request);

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol ahhhhhh! #205 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops.

Copy link
Contributor

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.

Copy link
Member

@devinivy devinivy Apr 24, 2021 •
edited
Loading

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

Copy link
Contributor

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?

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
};

Expand Down
53 changes: 52 additions & 1 deletion test/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1117,7 +1117,7 @@ describe('Manager', () => {
context: function (request) {

return {
message: request ? request.route.path : 'default message',
damusix marked this conversation as resolved.
Show resolved Hide resolved
message: 'default message',

query: {
test: 'global'
Expand All @@ -1132,6 +1132,57 @@ describe('Manager', () => {
expect(rendered).to.contain('<h1>default message</h1>');
});

it('renders with an async global context function (no request)', async () => {

const asyncFn = async () => await 'from async';

const server = Hapi.server();
const testView = new Manager(server, {
engines: { html: require('handlebars') },
path: __dirname + '/templates',

context: async function (request) {

return {
message: 'default message',

query: {
test: await asyncFn()
}
};
}
});

const rendered = await testView.render('valid/testContext');
expect(rendered).to.exist();
expect(rendered).to.contain('<h1>from async</h1>');
expect(rendered).to.contain('<h1>default message</h1>');
});

it('renders with a global context function that returns a promise (no request)', async () => {

const renderPromise = Promise.resolve({
message: 'default message',

query: {
test: 'from promise'
}
});

const server = Hapi.server();
const testView = new Manager(server, {
engines: { html: require('handlebars') },
path: __dirname + '/templates',

context: () => renderPromise
});

const rendered = await testView.render('valid/testContext');
expect(rendered).to.exist();
expect(rendered).to.contain('<h1>from promise</h1>');
expect(rendered).to.contain('<h1>default message</h1>');
});

it('overrides the global context function values with local values', async () => {

const server = Hapi.server();
Expand Down