Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Apr 11, 2024

Conversation

autopulated
Copy link
Contributor

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

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening a PR! Can you please add a unit test?

@autopulated
Copy link
Contributor Author

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
Copy link
Member

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

package.json Outdated Show resolved Hide resolved
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 8a78eb9 into fastify:master Apr 11, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants