No module augmentation #471
Conversation
| expectType<FastifyPluginAsync<FastifyStaticOptions, Server>>( | ||
| fastifyStaticStar.fastifyStatic | ||
| ) | ||
| app.register(fastifyStatic, { root: '/' }) |
There was a problem hiding this comment.
If I properly understand the change, we now require an object to be always passed when we register a fastify plugin (so it's no longer optional). This is a breaking change, we should really careful if we want to ship it as it is, since it will break existing TS codebases
There was a problem hiding this comment.
An object is not necessary, fastify static plugin has a mandatory root param. Removing the object leads to
Overload 1 of 3, (plugin: FastifyPluginCallback<FastifyPluginOptions, BaseFastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, FastifyTypeProviderDefault, FastifyDecorators>>, opts?: FastifyRegisterOptions<...> | undefined): BaseFastifyInstance<...> & ... 1 more ... & { ...; }, gave the following error.
Argument of type FastifyStaticPlugin<BaseFastifyInstance<any, any, any, any, any, FastifyDecorators>> is not assignable to parameter of type FastifyPluginCallback<FastifyPluginOptions, BaseFastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, FastifyTypeProviderDefault, FastifyDecorators>>.
Types of parameters opts and opts are incompatible.
Property root is missing in type FastifyPluginOptions but required in type FastifyStaticOptions.
...
Making root optional fixes the error.
If anything, the possibility of not providing root was a bug as far as i can tell.
| @@ -129,20 +138,24 @@ const appWithHttp2 = fastify({ http2: true }) | |||
|
|
|||
| appWithHttp2 | |||
| .register(fastifyStatic, options) | |||
| .after(() => { | |||
| appWithHttp2.get('/', (request, reply) => { | |||
| .after((err, instance) => { | |||
There was a problem hiding this comment.
This is a breaking change since we no longer support the scenario of the test: if err and instance params are not defined, .sendFile will not exist at TS level
There was a problem hiding this comment.
I do not see which test case this is, the code is obscured with the error message. Can you provide a test case that i can take a look at?
There was a problem hiding this comment.
It refers to the test case on line 140.
Basically, with the change, this test should still pass
WIP #5093
Checklist
npm run testandnpm run benchmarkand the Code of conduct