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

primeare commented 9 months ago

Prerequisites

[X] I have written a descriptive issue title
[X] 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.

primeare commented 9 months ago

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 commented 9 months ago

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

primeare commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

primeare commented 9 months ago

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 commented 9 months ago

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

primeare commented 9 months ago

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 commented 5 months ago

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

mweberxyz commented 5 months ago

After https://github.com/fastify/point-of-view/pull/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 commented 4 months ago

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

mweberxyz commented 4 months ago

@mcollina Between this issue and #412, I think best solution (for both) would be your original direction suggested in https://github.com/fastify/point-of-view/issues/394#issuecomment-1745156912

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 commented 4 months ago

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.

mweberxyz commented 3 months ago

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.

fastify / point-of-view