Broken Typescript types when defining options and done

[StarpTech]

Prerequisites

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

Fastify version

4.18.0

Plugin version

4.5.0

Node.js version

18

Operating system

Linux

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

-

Description

I can't define a plugin with options and done callback

import fp from "fastify-plugin";

export default fp(function Health(fastify, opts, done) {
  fastify.get("/health", (req, res) => {
    res.send();
  });
});

Results in TS7006: Parameter 'opts' implicitly has an 'any' type.

Steps to Reproduce

See description.

Workaround:

import fp from "fastify-plugin";
import { FastifyPluginCallback } from "fastify";

const plugin: FastifyPluginCallback = function Health(fastify, opts, done) {
  fastify.get("/health", (req, res) => {
    res.send();
  });

  done();
};

export default fp(plugin);

Expected Behavior

No response

[mcollina]

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

[StarpTech]

Sure, I think I can create a PR this weekend.

[StarpTech]

I spent some time on it. It seems the function type overloads are too dynamic and TS is not able to infer the right type, therefore fallbacks to any. Any reason why the plugin isn't implemented in TS directly? That would make the maintenance easier.

[kibertoad]

fastify ecosystem doesn't use direct TS for anything

[StarpTech]

Is it a rule? I'd rethink it.

[kibertoad]

I think @mcollina is concerned with compiled code potentially being less performant, but I have never seen any properly done research pointing out such perf degradation.

[StarpTech]

I understand it and it should be benchmarked but for those plugins like this one, it is negotiable because it's not in the hot path.

[mcollina]

Here are a few sparsed notes:

1. Fastify has a few TS modules, prominently fastify-passport is all written in TypeScript
2. TypeScript produces code that is slower than hand-crafted JS. Loading plugins is in the hot path for startup.
3. Everything that is possible by writing some code in TypeScript is possible by writing the types. I don't see how rewriting this module in TS would fix the problem.

Last but not least, the more I look at it, the more I think that any is the correct type here. What other type would you have?

[kibertoad]

TypeScript produces code that is slower than hand-crafted JS. Loading plugins is in the hot path for startup.

Are there any benchmarks for same code that support this? All that I ever saw amounted "we wrote a roughly similar thing in TS from a scratch, and hey, it ended up being slower, but we didn't even compare emited source". Are there any reasons why generated JS would be slower than exact same handwritten JS? Imports are likely to differ, but that's about it.

[Uzlopak]

Simple example:

https://www.typescriptlang.org/play?target=9#code/KYOwrgtgBAIglgJ2AYwC5wPYgKLmgbwFgAoKKAVQAcoBeKAcivoBoSyYMB3EWhj7lmygAZYADNUveqImDSUAEpwA5gAtJdekrWo5AXxIlkWAM4YANsAB05jMoAU8JGkw48VqgEpDxOCFTACGIAhsjAsIgo6FhQRPJUAFwMTKzy-CBJ9OlyZDKomXk5iirqmdrq+j7GICaSTlGuAPIARgBWUUn1LjF0cWSJyZRF6ZnZqbni+QyF48U6ZSW6qQbERqYW1rYOXdEgLe1oHpSeQA

[kibertoad]

@Uzlopak But you can use either of the two approaches in TS! And with second one you should get exact same perf as in JS.

[Uzlopak]

Sure. But you need this knowledge about ts to make an informed decision about which approach you want to use.

When I started with TS, I liked the fact that I just have to define the enum and have types and the runtime code covered. With the second approach, I have to define the interface and the object separately, making it double the work.

And to explain somebody that enums are bad because enums are transpiled to that does not convince an usual typescript dev.

[StarpTech]

Last but not least, the more I look at it, the more I think that any is the correct type here. What other type would you have?

No, it's not. The arguments of both example has to work.

// Integration tests

// Here all arguments are of type any
const pluginCallbackWithOptionsIntegration = fastifyPlugin<Options>(function (fastify, options, next) {
  expectAssignable<Options>(options)
  expectAssignable<(err?: Error) => void>(next)
})
// works
const pluginAsyncWithOptionsIntegration = fastifyPlugin<Options>(async function (fastify, options) {
  expectAssignable<Options>(options)
})

Everything that is possible by writing some code in TypeScript is possible by writing the types. I don't see how rewriting this module in TS would fix the problem.

You need to express that polymorphism manually instead of letting the compiler generate the code for you. I think it can be definitely fixed but it requires significant knowledge and this prevents contributions. If the goal is to provide first-class typescript support, I'd reevaluate the approach to writing code in Typescript. I don't believe it will have significant performance drops and if, we can optimize and benchmark it.

[Uzlopak]

What you mean with polymorphism?

[StarpTech]

https://github.com/fastify/fastify-plugin/blob/master/types/plugin.d.ts#L48-L82

[StarpTech]

Sure. But you need this knowledge about ts to make an informed decision about which approach you want to use. When I started with TS, I liked the fact that I just have to define the enum and have types and the runtime code covered. With the second approach, I have to define the interface and the object separately, making it double the work. And to explain somebody that enums are bad because enums are transpiled to that does not convince an usual typescript dev.

If performance really matters, we should add benchmarks to this repository and don't solely trust that people can write good idiomatic javascript code.

[mcollina]

If performance really matters, we should add benchmarks to this repository and don't solely trust that people can write good idiomatic javascript code.

Not really on this module if we are writing it in JS: it's very easy to review. Ultimately TS can generate a lot of bloat that can be unexpected (the wrong option set can trigger it).

If we ever decide to turn this into TS we would need benchmarks.

[kibertoad]

We control the option set, though. Can create tightly reviewed standard fastify one.

[mcollina]

You need to express that polymorphism manually instead of letting the compiler generate the code for you. I think it can be definitely fixed but it requires significant knowledge and this prevents contributions. If the goal is to provide first-class typescript support, I'd reevaluate the approach to writing code in Typescript. I don't believe it will have significant performance drops and if, we can optimize and benchmark it.

The argument I might stand behind is about lowering the contribution barrier. However, this has nothing to do with this specific bug.

I don't think it's possible to write Fastify in TypeScript with the current semantics around plugins. Ultimately it would be a very different framework, with likely too many breaking changes. (I'm not sure who would fund that work even).

I'm -1 to rewrite this module in TS to fix this bug. The discussion on the usage of TS across is worth having on a separate issue.

[Uzlopak]

created a PR #218 to get atleast the error visible.

It seems that there is an issue to detect the correct type of the passed fn to fastify-plugin-function.

[kibertoad]

@StarpTech Can you create a separate issue for TS conversion? I would be +1 on this and can help with benchmarking.

[StarpTech]

Yeah, sorry I mixed the topics a bit. Let's focus here on how to solve the type issue specifically.

[Uzlopak]

@StarpTech please have a look at the PR #218