fastify / point-of-view

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

suggestion: await engine to support inline engine import in options #422

Closed autopulated closed 3 months ago

autopulated commented 3 months ago

With commonjs it was common to require the engine in the options, and the current readme suggests this usage:

fastify.register(require("@fastify/view"), {  engine: { ejs: require("ejs") } });

With modules, the same usage wasn't previously possible, because the inline import returns a promise.

app.register(import('@fastify/view'), { engine: { ejs: import('ejs') } })
// results in error later that "engine.compile is not a function"

So instead you had to await that import inline, and delay setting up further plugins:

app.register(import('@fastify/view'), { engine: { ejs: await import('ejs') } })

Since the plugin initialisation is already an async function, it seemed straightforward to just await the engine during plugin initialisation instead, which will hopefully allow more plugin loading to occur before blocking on the await.

Is this a good idea? If so I will update the docs as well.

If not we could add validation that the engine is actually an engine (has a .compile function?), to catch this error at plugin initialisation time?

Checklist

autopulated commented 3 months ago

I've added a test using a .mjs file in the tests directory (had to explicitly point tap at this).

I'm not sure if this will cause a problem since everything was previously commonjs. If it is then the test could just use require and manually wrap the module in a promise instead?

gurgunday commented 3 months ago

I think import() exists in CJS and does the same thing

Ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import

But this is fine