Proposal: Remove `send` calls

[mweberxyz]

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched existing issues to ensure the issue has not already been raised

Issue

I'd like to propose a semver-major change: remove remaining calls to send throughout the plugin. Instead, the async reply decorator would always return a promise resolving to the rendered html.

This would necessitate docs, test, and code changes. It would require a prominent "Breaking Changes" to be added to README.md.

Pros

• Error handling: Any errors generated internally by either fs operations or the renderers themselves could be handled by developer's defined setErrorHandler, or tracked by onError hooks
• Consistency: Both async and traditional handlers would be required to return the render result
• Single entry point: the viewDecorator could be removed, except to expose the existing clearCache functionality. This would ensure all calls to the renderer set this to a valid request object (resolving #394)

Cons

• Would probably break a significant number of apps created by users of this plugin

Samples

README.md - Quick start

fastify.get("/", (req, reply) => {
  return reply.view("/templates/index.ejs", { text: "text" });
});

Alternatives

As a less-invasive alternative, perhaps a reply decorator such as ${propertyName}Safe or ${propertyName}AsPromise could be implemented, with all of the semantics described above, and keep the existing reply decorator as-implemented.

[Uzlopak]

Are you referring to calls to @fastify/send?

[Uzlopak]

Scratch that, we dont use this in this repo.

[mweberxyz]

Essentially, the implementation would be changing this:

  fastify.decorateReply(propertyName, async function (page) {
    if (!page) {
      this.send(new Error('Missing page'))
    }
    try {
      const result = await renderer.apply(this, arguments)
      if (!this.getHeader('Content-Type')) {
        this.header('Content-Type', 'text/html; charset=' + charset)
      }

      if (minify && !isPathExcludedMinification(this)) {
        this.send(minify(result, globalOptions.htmlMinifierOptions))
      } else {
        this.send(result)
      }
    } catch (err) {
      this.send(err)
    }
    return this
  })

to this:

  fastify.decorateReply(propertyName, async function (page) {
    if (!page) {
      throw new Error('Missing page')
    }

    const promise = renderer.apply(this, arguments)
      .then((result) => {
        if (!this.getHeader('Content-Type')) {
          this.header('Content-Type', 'text/html; charset=' + charset)
        }
        return result
      })

    if (minify && !isPathExcludedMinification(this)) {
      return promise.then((result) => minify(result, globalOptions.htmlMinifierOptions))
    }

    return promise
  })

Writing it out I realize a few things:

• Moving the minifier call to .then would solve #372
• Benchmark-wise - this might be a net gain (due to the removal of the await in the call to renderer.apply), or a net loss due to the additional overhead of the .then closures. If there is any appetite for this change, I can push forward enough to get some numbers.

[mcollina]

Can you put it behind a flag for backward compatibility?