fastify / point-of-view

Template rendering plugin for Fastify
MIT License
338 stars 86 forks source link

Errors in eta template usinc async mode are not catched #346

Closed cberescu closed 1 year ago

cberescu commented 1 year ago

Prerequisites

Fastify version

4.6.0

Plugin version

7.1.0

Node.js version

16.16

Operating system

Linux

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

20.04

Description

In case of using the ETA engine and an error is inside the template there is no way to catch it in async mode

Steps to Reproduce

An example of the kind of error i am referring to :

undefined:14
tR+=E.e(it.personalInfo.birthday ? it.personalInfo.birthday.toISOString().split('T')[0] : '')
                                                            ^

RangeError: Invalid time value
    at Date.toISOString (<anonymous>)
    at eval (eval at compile (/app/node_modules/eta/dist/eta.cjs:595:16), <anonymous>:14:61)
    at tryHandleCache (/app/node_modules/eta/dist/eta.cjs:824:6)
    at renderFile (/app/node_modules/eta/dist/eta.cjs:926:10)
    at Object.renderFileAsync (/app/node_modules/eta/dist/eta.cjs:934:12)
    at Object.viewEta (/app/node_modules/@fastify/view/index.js:676:8)
    at /app/node_modules/@fastify/view/index.js:94:15
    at new Promise (<anonymous>)
    at Object.viewDecorator [as view] (/app/node_modules/@fastify/view/index.js:91:21)

Expected Behavior

The try {} catch {} to work

Eomm commented 1 year ago

Could you add a Minimal, Reproducible Example?

Without it, we are unable to help you.

cberescu commented 1 year ago

sure, here u have an example : https://www.dropbox.com/s/4go3qq6lzpu2dfj/fastview.zip?dl=0

Eomm commented 1 year ago

Sorry, could you commit the source code to a GitHub repository?

It is unsafe downloading a zip file

cberescu commented 1 year ago

sure : https://github.com/cberescu/fasttest

Eomm commented 1 year ago

Yeah, I think it is a bug

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

cberescu commented 1 year ago

Unfortunately no, i can’t figure out where the issues is exactly

gurgunday commented 1 year ago

Honestly, I don't think it's got to do with the async option here: the error isn't caught even if async is set to false.

await fastify.register(import("@fastify/view"), {
  engine: {
    eta: await import("eta"),
  },
  root: new URL("views", import.meta.url).pathname,
  options: {
    // async: true,
  },
});

// Does not return Error Found in case of error
fastify.get("/", async (request, reply) => {
    try {
    return reply.view("index.eta", { data: false });
  } catch (error) {
    return "Error Found";
  }
});
cberescu commented 1 year ago

It works correctly on my side, only the async crashes. I am starting to think it is an eta problem specifically from : getAsyncFunctionConstructor

import Fastify from 'fastify'
import * as path from 'path';
const fastify = Fastify({
  logger:true,
  ignoreTrailingSlash: true,
  maxParamLength: 300,
  trustProxy: true
});
await fastify.register(import("@fastify/view"), {
  engine: {
    eta: await import("eta"),
  },
  root: path.resolve('web'),
  options: {
     // async: true,
  },
});

// Does not return Error Found in case of error
fastify.get("/", async (request, reply) => {
    try {
    return reply.view("view.eta", { data: false });
  } catch (error) {
    console.log(error);
    return "Error Found";
  }
});

fastify.listen({port: process.env.PORT || 80, host:"0.0.0.0"} , (err, address) => {

  if (err) throw err

})
cberescu commented 1 year ago

So, my bad, the issue is from the plugin, i used ETA directly without the plugin and works correctly. I updated the repository with the example.

gurgunday commented 1 year ago

Okay, I see: this happens because of the structure of the plugin. Right now, correct me if I'm wrong, the error crashes the server, right? For that, I will open a PR that will align this error case with the rest of the API.

BUT, the bigger reason why it's won't respect the trycatch, even after my PR, is that if an error is thrown inside the view functions, they simply send the error back to the user using reply.send— this seems to be hard coded at the moment.

Since most template engines return the page in the form of a string in the end, a better way to handle this could be if the reply.view function only returned the final template as a string while setting the reply header as HTML, and since we already return it when using the async API, it would be sent back to the user.

I wanted to heavily refactor point-of-view anyway, so I might try this when I have the time.

cberescu commented 1 year ago

Yes, it crashes the server, for now i was able to find where the issue is and pull requested a fix . I didn't know you started to work on one also. Should have said i was working on one but wasn't sure I will be able to find the issue.

gurgunday commented 1 year ago

It's fine - as long as Fastify improves I'm happy ;)