`fastify.view` is rendered without `reply.locals` about point-of-view HOT 18 OPEN

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.

from point-of-view.

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

from point-of-view.

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

from point-of-view.

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.

from point-of-view.

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?

from point-of-view.

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.

from point-of-view.

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:

1. 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 });
});

2. Deprecate fastify.view in the semver-minor version and advise using the new reply.view with the renderOnly option.
3. 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.

from point-of-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.

Can you explain why?

from point-of-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.

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?

from point-of-view.

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

from point-of-view.

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?

from point-of-view.

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

from point-of-view.

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?

from point-of-view.

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

from point-of-view.

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.

from point-of-view.

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

from point-of-view.

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

from point-of-view.

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.

from point-of-view.