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.

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

feat: viewAsync reply decorator #420

Merged
merged 4 commits into from
Apr 22, 2024
Merged

Conversation

mweberxyz
Copy link
Contributor

@mweberxyz mweberxyz commented Mar 22, 2024

Closes #394 and closes #412

The purpose of this PR is to implement a new reply decorator, called viewAsync by default.

viewAsync is similar to view, except it does not call reply.send on template render or error, and instead returns a promise (to be returned from the route handler and allow fastify hook chain to take over).

See #394 and #412 for context. 412 will be closed by this PR, though maybe it should be revisited in the future, if the legacy view should be replaced with viewAsync as the default - would be a semver-major change and would probably make most sense to coincide with a major fastify version bump.

Approach

  1. Add new asyncRender async function
  2. Add new fastify.viewAsync decorator
  3. Call asyncRender for existing fastify.view decorator
  4. Call asyncRender for existing reply.view decorator

With this PR, all exposed decorators utilize the same underlying implementation of the rendering function, so each only contains logic specific to the needs of that decorator.

Tests

Sufficient tests added to be thoroughly confident the changes work as expected. Of particular interest:

  1. EJS: Tested both sync and async route handlers
  2. EJS: Ensured a call to fastify's setErrorHandler works as expected
  3. EJS: Tested asyncPropertyName and propertyName options work as expected
  4. Handlebars: Added a test specifically to make sure fastify.view is rendered without reply.locals #394 is resolved

Benchmarks

  • added fastify-viewAsync, identical to fastify.js but uses reply.viewAsync instead of reply.view

All within run-to-run variance (my laptop, M1 mac @ Node v20):

Benchmark PR Master
express.js 20354.67 req/s 20104 req/s
fastify.js 110634.67 req/s 112906.67 req/s
fastify-art.js 23810.67 req/s 23501.34 req/s
fastify-dot.js 128826.67 req/s 128192 req/s
fastify-eta.js 80298.67 req/s 81258.67 req/s
fastify-pug.js 118080 req/s 120672 req/s
fastify-twig.js 96906.67 req/s 98037.34 req/s
fastify-liquid.js 24936 req/s 25080 req/s
fastify-mustache.js 89386.67 req/s 88512 req/s
fastify-nunjucks.js 113600 req/s 113162.67 req/s
fastify-ejs-async.js 108789.34 req/s 111978.67 req/s
fastify-viewAsync.js 112384 req/s -
fastify-ejs-minify.js 35445.34 req/s 35360 req/s
fastify-handlebars.js 84170.67 req/s 83808 req/s
fastify-ejs-local-layout.js 96746.67 req/s 97888 req/s
fastify-ejs-global-layout.js 99914.67 req/s 99754.67 req/s

Docs

Cleanup, re-organize for readability, and add documentation for new decorator:

  • Options list to table
  • Quick start: add template to README.md, change async example to use viewAsync
  • Added more filled out Layouts section with instructions for how to use them.
  • Moved Providing a layout on render to be before Rendering the template into a variable, so the former can reference the latter and it makes sense reading from top-to-bottom
  • Added Migrating from view to viewAsync
  • Pulled much of the charset note at bottom out, add charset to options table

Checklist

@mweberxyz
Copy link
Contributor Author

mweberxyz commented Mar 22, 2024

@mweberxyz mweberxyz changed the title feat: asyncView feat: viewAsync Mar 22, 2024
@mweberxyz mweberxyz changed the title feat: viewAsync feat: viewAsync reply decorator Mar 22, 2024
@mweberxyz mweberxyz force-pushed the feat/view-async branch 2 times, most recently from e8e6dbe to 28d001c Compare March 22, 2024 16:19
@mweberxyz
Copy link
Contributor Author

mweberxyz commented Mar 22, 2024

The only difference between fastify.view and reply.viewAsync when called to render the html into a string inside a route handler is that the Content-Type: text/html.. header will be set by the latter, if Content-Type has not already been set.

@mweberxyz mweberxyz marked this pull request as draft March 23, 2024 16:12
@mweberxyz mweberxyz force-pushed the feat/view-async branch 2 times, most recently from c3f2905 to 112d60c Compare March 23, 2024 16:42
@mweberxyz mweberxyz marked this pull request as ready for review March 23, 2024 16:44
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@mweberxyz
Copy link
Contributor Author

This has been sitting for awhile - anything needed to get it merged? It's semver-minor due to purely additive functionality.

index.js Outdated
Comment on lines 128 to 130
const fakeRequest = {
getHeader: () => true
}
Copy link
Member

Choose a reason for hiding this comment

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

Why a fake request?

Copy link
Contributor Author

@mweberxyz mweberxyz Apr 12, 2024

Choose a reason for hiding this comment

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

It's what this is bound to in the call to asyncRender by the old viewDecorator on line 140 -- because viewDecorator does not have this set to a request.

index.js Outdated
if (minify) {
promise = promise.then((result) => minify(result, globalOptions.htmlMinifierOptions))
}
const promise = asyncRender.apply(fakeRequest, args)
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for a fakeRequest, you can use a real one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no request in scope, it's the crux of #394 -- this is fastify.view not reply.view.

If it seems hacky, I can drop it and add the conditional into the calls to this.getHeader() on line 121?

My intent here was to avoid adding a branch into the critical path of the plugin to avoid performance regression.

Copy link
Contributor Author

@mweberxyz mweberxyz Apr 13, 2024

Choose a reason for hiding this comment

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

Or I could just construct thisArg on every request like used to happen here: 235f588#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L857 if that's more clear/maintainable.

Same intent: avoid creating object unnecessarily in critical path for performance reasons, though in this case it would be less impactful as this would only in the critical path of fastify.view as opposed to everything. lmk what you all prefer.

index.js Outdated
Comment on lines 121 to 123
if (!this.getHeader('Content-Type')) {
this.header('Content-Type', 'text/html; charset=' + charset)
}
Copy link
Member

@gurgunday gurgunday Apr 15, 2024

Choose a reason for hiding this comment

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

Oh I get what you mean now, seems a little hacky but performant I guess :)

Can you please try this instead of fakeRequest to see if it impacts performance? It could maybe have an impact if we call this method multiple times

Suggested change
if (!this.getHeader('Content-Type')) {
this.header('Content-Type', 'text/html; charset=' + charset)
}
this.header?.('Content-Type', 'text/html; charset=' + charset)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't do that exactly because existing logic/tests ensure header is only set if not already set. Pushed something along these lines after confirming no regression is benchmarks.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the change, it is easier to understand for me

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -44,6 +46,7 @@ declare namespace fastifyView {
root?: string;
viewExt?: string;
propertyName?: string;
asyncPropertyName?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for this type? We use tsd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ptal at most recent commit

@mcollina mcollina merged commit cb855ed into fastify:master Apr 22, 2024
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Remove send calls fastify.view is rendered without reply.locals
3 participants