`fastify.view` is rendered without `reply.locals` #394

primeare · 2023-10-01T19:04:51Z

Prerequisites

I have written a descriptive issue title
I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.23.2

Plugin version

8.2.0

Node.js version

20.8.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.0 (23A344)

Description

There is a possibility to render the template into a variable (as described here) using fastify.view, or in my case, fastify.render as I renamed the property using "propertyName" option. I use Handlebars as an engine. I also have a pre-handler set in one of my custom plugins to extend reply.locals (as described here).
The problem is that fastify.view renders the template without passing reply.locals, whereas reply.view renders the template as expected with reply.locals.

Steps to Reproduce

Application

import fastifyView from '@fastify/view';
import fastify from 'fastify';
import fastifyPlugin from 'fastify-plugin';
import handlebars from 'handlebars';
import { randomUUID } from 'node:crypto';
import { dirname, join as joinPath } from 'node:path';
import { fileURLToPath } from 'node:url';

const filePath = fileURLToPath(import.meta.url);
const templatesDirPath = joinPath(dirname(filePath), 'templates');

const app = fastify({
  ignoreTrailingSlash: true,
  onProtoPoisoning: 'error',
  onConstructorPoisoning: 'error',
  return503OnClosing: false,
  connectionTimeout: 10_000, // 10 seconds
  requestTimeout: 30_000, // 30 seconds
  pluginTimeout: 10_000, // 10 seconds
  genReqId: () => randomUUID(),
  logger: {
    level: 'trace',
    transport: {
      target: 'pino-pretty',
      options: {
        translateTime: 'HH:MM:ss Z',
        ignore: 'pid,hostname',
      },
    },
  },
  ajv: {
    customOptions: {
      allErrors: false,
      validateFormats: true,
      strictNumbers: true,
    },
  },
});

app.register(fastifyView, {
  engine: { handlebars },
  root: templatesDirPath,
  propertyName: 'render',
  viewExt: 'hbs',
  layout: './layouts/main',
  includeViewExtension: true,
  defaultContext: {},
  options: {},
});

app.register(fastifyPlugin(async (fastify) => {
  fastify.addHook('preHandler', (request, reply, done) => {
    const path = request.url.endsWith('/') ? request.url : request.url + '/';
    const full = `${request.protocol}://${request.hostname}${path}`;

    reply.locals = {
      ...reply.locals,
      url: { path, full },
    };

    done();
  });
}, {
  name: 'my-plugin',
  dependencies: ['@fastify/view'],
}));

app.route({
  method: 'GET',
  url: '/',
  async handler(request, reply) {
    console.log('Locals: ', reply.locals);
    const html = await this.render('page', {}); // as opposite to `reply.render`
    reply.type('text/html');
    return html;
  }
});

await app.ready();

app.listen({
  port: 3000,
  host: '0.0.0.0',
}, (err) => {
  if (err) {
    app.log.fatal(err);
    return process.exit(1);
  }

  console.log('\nServer Plugins:');
  console.log(app.printPlugins());
  console.log('Server Routes:');
  console.log(app.printRoutes({ includeHooks: false, commonPrefix: false }));
});

Layout

<!DOCTYPE html>
<html lang="en">
<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <title>Document</title>
</head>
<body>
  {{{body}}}
</body>
</html>

Template

<h1>URL: {{url.full}}</h1>

Expected Behavior

Rendering with fastify.view should work the same way as reply.view except it returns the rendering result without sending it.

The text was updated successfully, but these errors were encountered:

primeare · 2023-10-02T09:50:14Z

Well, a quick workaround to the problem would be to pass reply.locals in handler as data:

async handler(request, reply) {
  console.log('Locals: ', reply.locals);

  const html = await this.render('page', {
    ...reply.locals,
  });

  reply.type('text/html');
  return html;
}

But it would be much better to be implemented out of the box if possible

mcollina · 2023-10-02T10:55:15Z

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

primeare · 2023-10-02T12:20:20Z

As I was reading the source code, I found the difference lies behind the decoration in Fastify. When a reply is decorated with "view" and "locals", they are in the same Reply class, but that's not the case for decoration of Fastify instance with "view". So, probably, this issue should be resolved in Fastify, but I'm not a big fan of coupling two packages together this way. I see a possibility to get Fastify's "Reply Symbol" in this package like so:

const ReplySymbol = Object.getOwnPropertySymbols(this).find((symbol) => {
  return symbol.description === 'fastify.Reply';
});

And then use it to retrieve locals from reply of Fastify's instance. Though, a proper benchmark is needed for such a code. I would be glad to discuss a better solution if anyone sees it.

Anyway, I think Fastify's point-of-view should provide the same behaviour for fastify.view and reply.view as there are cases when you need to render template first and then build a reply.

UPD. fastify.Reply symbol will not help retrieve the reply instance from Fastify. So that's a false path.

primeare · 2023-10-02T12:24:15Z

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

I would be glad to help and make a pull request. Do you know if it is possible to get a reply somehow in the plugin?

mcollina · 2023-10-03T09:47:42Z

You can't, that's the key difference between the two APIs. However, we could add a way to pass a reply in to the one on the instance.

primeare · 2023-10-03T11:22:23Z

I was researching quite a lot about how we can get or pass an actual reply to the plugin. Unfortunately, I haven't figured out any good solution without hacks, although it is possible. However, I had a thought that we can do the following:

Introduce an option renderOnly to the reply.view so that it would behave the same as fastify.view. The only issue is that we need to change the function contract for this case. If the renderOnly option is passed, the reply.view function returns a rendering result. Otherwise, it behaves the same as before returning this, which is Fastify's reply instance. In such a way, we can guarantee backwards compatibility.

fastify.get('/', (request, reply) => {
  const html = await reply.view('index-for-layout.hbs', data, { renderOnly: true });
});

Deprecate fastify.view in the semver-minor version and advise using the new reply.view with the renderOnly option.
Remove fastify.view in the semver-major version.

@mcollina, what do you and the community think about that?
From my point of view, this would improve the codebase by removing the duplicate functionality and would make it more apparent from the API standpoint of the behaviour expected from the function.

mcollina · 2023-10-03T13:27:00Z

The only issue is that we need to change the function contract for this case. If the renderOnly option is passed, the reply.view function returns a rendering result.

Can you explain why?

primeare · 2023-10-03T13:42:46Z

The only issue is that we need to change the function contract for this case. If the renderOnly option is passed, the reply.view function returns a rendering result.

Can you explain why?

Currently, reply.view always returns this, which is Fastify's reply instance (index.js#L124). If we want to implement "render to variable" functionality, we need to change the interface slightly so that if you pass renderOnly option to the reply.view, it outputs Promise with the rendering result. Did I make it clear?

mcollina · 2023-10-03T13:43:53Z

I would actually prefer not to have different result for the function based on the option. Maybe add a fresh one?

primeare · 2023-10-03T14:49:23Z

That would work. We can decorate the reply with an additional function that renders to a variable, in lieu of fastify.view. But then how to name it better and how to deal with the "propertyName" configuration option?

mcollina · 2023-10-03T14:58:32Z

how about ${propertyName}Render? or another option.

primeare · 2023-10-04T17:40:12Z

how about ${propertyName}Render? or another option.

Taking into account how people use propertyName: 'render' or other variations (see this GitHub Code Search), I think we should either find a universal naming or a different approach. Maybe ${propertyName}OnlyRender would work or give users a configuration option for this property?

mweberxyz · 2024-02-16T16:32:29Z

Another issue of current approach is the pathsToExcludeHtmlMinifier feature does not work when invoked via fastify.view.

mweberxyz · 2024-02-17T17:05:04Z

After #405 merges this could be solved with a documentation change.

IE the following will work as expected:

fastify.get('/', async (req, reply) => {
  reply.locals.text = 'text'
  const html = await fastify.view.call(reply, 'index.ejs')
  return html
})

Happy to create new PR with documentation change and tests after 405 merges if this is a suitable solution.

mcollina · 2024-03-21T15:48:51Z

@mweberxyz would you mind to se4nd a PR for the doc change?

mweberxyz · 2024-03-21T16:34:35Z

@mcollina Between this issue and #412, I think best solution (for both) would be your original direction suggested in #394 (comment)

We can keep both fastify.view and reply.view implementation without change, and implement a new reply decorator that satisfies the suggestions in #412 -- removing calls to send and instead returning a promise that resolves to rendered HTML.

I propose ${propertyName}Async as the name of the decorator - thoughts? By default that would be reply.viewAsync, or (using the samples from the README) reply.mobileAsync/reply.desktopAsync. This feels unlikely to conflict with any names people have named their decorators.

primeare · 2024-03-21T17:30:23Z

I propose ${propertyName}Async as the name of the decorator - thoughts? By default that would be reply.viewAsync, or (using the samples from the README) reply.mobileAsync/reply.desktopAsync. This feels unlikely to conflict with any names people have named their decorators.

I like the idea of appending Async to the new refactored functions. In the worst case, people will get something like renderAsyncAsync if they have chosen to use the renderAsync property name. But I assume this is unlikely. Anyway, docs should be updated to inform developers about this, and we should encourage developers to transition to the new functions.
Additionally, we may introduce something like asyncPropertyName configuration, defaulting to viewAsync or renderAsync or ${propertyName}Async, and it will allow developers to set the preferred name for these two functions manually. For instance, this will enable them to set propertyName to deprecatedRender and asyncPropertyName to render so in the result, they only have to change the async flow in their code, but not function contracts.

Closes fastify#394 Closes fastify#412

mweberxyz · 2024-03-26T15:50:17Z

I didn't realize every time I force push it does this to the issue 😳, will try to catch mistakes before creating PR in future.

* feat: viewAsync reply decorator Closes #394 Closes #412 * remove fakeRequest * add type tests --------- Signed-off-by: Matteo Collina <hello@matteocollina.com> Co-authored-by: Matteo Collina <hello@matteocollina.com>

mweberxyz mentioned this issue Feb 19, 2024

Proposal: Remove send calls #412

Closed

2 tasks

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 22, 2024

feat: asyncView

1d82d5d

Closes fastify#394 Closes fastify#412

mweberxyz mentioned this issue Mar 22, 2024

feat: viewAsync reply decorator #420

Merged

4 tasks

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 22, 2024

feat: viewAsync reply decorator

e8e6dbe

Closes fastify#394 Closes fastify#412

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 22, 2024

feat: viewAsync reply decorator

28d001c

Closes fastify#394 Closes fastify#412

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 22, 2024

feat: viewAsync reply decorator

76e703b

Closes fastify#394 Closes fastify#412

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 23, 2024

feat: viewAsync reply decorator

c3f2905

Closes fastify#394 Closes fastify#412

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 23, 2024

feat: viewAsync reply decorator

112d60c

Closes fastify#394 Closes fastify#412

mweberxyz added a commit to mweberxyz/point-of-view that referenced this issue Mar 23, 2024

feat: viewAsync reply decorator

4007699

Closes fastify#394 Closes fastify#412

mcollina closed this as completed in #420 Apr 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`fastify.view` is rendered without `reply.locals` #394

`fastify.view` is rendered without `reply.locals` #394

primeare commented Oct 1, 2023 •

edited

Loading

primeare commented Oct 2, 2023

mcollina commented Oct 2, 2023

primeare commented Oct 2, 2023 •

edited

Loading

primeare commented Oct 2, 2023

mcollina commented Oct 3, 2023

primeare commented Oct 3, 2023 •

edited

Loading

mcollina commented Oct 3, 2023

primeare commented Oct 3, 2023

mcollina commented Oct 3, 2023

primeare commented Oct 3, 2023

mcollina commented Oct 3, 2023

primeare commented Oct 4, 2023

mweberxyz commented Feb 16, 2024

mweberxyz commented Feb 17, 2024 •

edited

Loading

mcollina commented Mar 21, 2024

mweberxyz commented Mar 21, 2024 •

edited

Loading

primeare commented Mar 21, 2024

mweberxyz commented Mar 26, 2024

fastify.view is rendered without reply.locals #394

fastify.view is rendered without reply.locals #394

Comments

primeare commented Oct 1, 2023 • edited Loading

Prerequisites

Fastify version

Plugin version

Node.js version

Operating system

Operating system version (i.e. 20.04, 11.3, 10)

Description

Steps to Reproduce

Application

Layout

Template

Expected Behavior

primeare commented Oct 2, 2023

mcollina commented Oct 2, 2023

primeare commented Oct 2, 2023 • edited Loading

primeare commented Oct 2, 2023

mcollina commented Oct 3, 2023

primeare commented Oct 3, 2023 • edited Loading

mcollina commented Oct 3, 2023

primeare commented Oct 3, 2023

mcollina commented Oct 3, 2023

primeare commented Oct 3, 2023

mcollina commented Oct 3, 2023

primeare commented Oct 4, 2023

mweberxyz commented Feb 16, 2024

mweberxyz commented Feb 17, 2024 • edited Loading

mcollina commented Mar 21, 2024

mweberxyz commented Mar 21, 2024 • edited Loading

primeare commented Mar 21, 2024

mweberxyz commented Mar 26, 2024

`fastify.view` is rendered without `reply.locals` #394

`fastify.view` is rendered without `reply.locals` #394

primeare commented Oct 1, 2023 •

edited

Loading

primeare commented Oct 2, 2023 •

edited

Loading

primeare commented Oct 3, 2023 •

edited

Loading

mweberxyz commented Feb 17, 2024 •

edited

Loading

mweberxyz commented Mar 21, 2024 •

edited

Loading