add Promise version overload for FastifyPlugin #2350
Conversation
types/plugin.d.ts
Outdated
| ): void; | ||
| export type FastifyPlugin<Options extends FastifyPluginOptions = {}, FI extends FastifyInstance = FastifyInstance> = | ||
| { | ||
| (instance: FI, opts: Options, next: (err?: FastifyError) => void): void; |
There was a problem hiding this comment.
I think the type for the err parameter should be Error.
There was a problem hiding this comment.
That's unchanged from how it was before.
I can change it here but I guess a separate PR would be better.
|
This looks good! I won’t be back at my laptop until Monday, so I can’t check it out locally. I’m not totally following what’s happening with the instance argument and FastifyInstance but I’ll be sure to revisit that on Monday 😄 |
A little clarification, there are 2 issues here
|
I'm checking this out locally and I see what you mean now. I'm going to dig in and try to figure this out. I've hit this before and I don't remember the solution. Have you tried opening a Stackoverflow question for it? I usually get good responses to my TypeScript questions |
|
Aha! microsoft/TypeScript#32571 - lets see what they reply. I don't know if there exists a work around at this time. Maybe your current generic inference is the best path but lets see. |
|
Another bit of relevant information of why the generics are defined as they are now: fastify/fastify-plugin#85 I believe it has to do with assumptions on external plugins This comment in particular might help here: fastify/fastify-plugin#85 (comment) Sorry there is so much to this, plugins have been the hardest thing to define types for 😅 -- I think I understand what you are trying to do with the instance inference. My assumption was that when creating a plugin the user would specify its type as interface TestOptions extends FastifyPluginOptions {
option1: string;
option2: boolean;
}
const testPlugin: FastifyPlugin<TestOptions> = function (instance, opts, next) { }To summarize and highlight the previous discussion, plugins should be written in a way that does not require the instance to be specified, and they should handle any of the possible standard instance configurations (http vs https vs http2). Correct me if I'm wrong, but i believe the generic inference you've implemented is a different way of achieving type casting i.e. you can achieve the same types by doing this in your implementation: fastify().register(function (_inst, opts, next) {
const instance = _inst as FastifyInstance
}))This is considered safer than the generic inference because its more obvious and intentional. I'm going to reject your change for specifying the instance type, but you're on to something with the promise one. And I think I have a work around in mind (will share in a new comment). |
types/plugin.d.ts
Outdated
| export type FastifyPlugin<Options extends FastifyPluginOptions = {}, FI extends FastifyInstance = FastifyInstance> = | ||
| { | ||
| (instance: FI, opts: Options, next: (err?: FastifyError) => void): void; | ||
| } | { |
There was a problem hiding this comment.
Lets break these apart into two separate types or interfaces. Call them FastifyPluginSync and FastifyPluginAsync. Update FastifyPlugin to be FastifyPluginSync | FastifyPluginAsync (pass through the options generic_, and make sure to export all three of these types. In the test file create a new test plugin that is specified by the FastifyPluginAsync type and pass it to register.
This should fix the async bug and enable users to safely type their plugins.
There was a problem hiding this comment.
Let's call them FastifyPluginCallback and FastifyPluginAsync. No one is sync.
There was a problem hiding this comment.
Will try this out, looks good.
@Ethan-Arrowood Completely agree that plugins should be agnostic to specific fastify instance (until such segregation is supported explicitly). |
This is intentional. |
|
@Ethan-Arrowood Oh, I've misunderstood the idea then. My intent was to just type Also, wdyt of adding a separate type of |
lundibundi
force-pushed
the
fix-plugin-fn-type
branch
from
de2be4b
to
2e84534
Compare
|
Hmm yes it does look like the cli template should be updated. If you'd like to send a PR for that too it would be appreciated! To be honest I'm not sure about a |
There was a problem hiding this comment.
Looking good! Can you try and replace the | operator with &
I got a simplified playground to work with this. I'm hoping it work for us here too!
|
@Ethan-Arrowood that breaks type inference for both callback and async (already broken) versions unfortunately: I think that's due to that async TS bug. |
|
|
|
One last trial: playground try this kind of function overloading please |
|
Not sure if I correctly understood the idea: diff --git a/test/types/plugin.test-d.ts b/test/types/plugin.test-d.ts
index b8c7f04..065719c 100644
--- a/test/types/plugin.test-d.ts
+++ b/test/types/plugin.test-d.ts
@@ -40,9 +40,9 @@ const testPluginAsync: FastifyPluginAsync = async function (instance, opts) { }
expectAssignable<FastifyInstance>(fastify().register(testPluginAsync, {}))
// TODO: Uncomment when https://github.com/microsoft/TypeScript/issues/32571 is resolved.
-// expectAssignable<FastifyInstance>(fastify().register(function (instance, opts): Promise<void> { }))
-// expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, () => { }))
-// expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, { logLevel: 'info', prefix: 'foobar' }))
+expectAssignable<FastifyInstance>(fastify().register(function (instance, opts): Promise<void> { return Promise.resolve() }))
+expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, () => { }))
+expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, { logLevel: 'info', prefix: 'foobar' }))
expectError(fastify().register(function (instance, opts, next) { }, { logLevel: '' })) // must use a valid logLevel
diff --git a/types/register.d.ts b/types/register.d.ts
index 5eda7bd..664eca9 100644
--- a/types/register.d.ts
+++ b/types/register.d.ts
@@ -1,4 +1,4 @@
-import { FastifyPlugin, FastifyPluginOptions } from './plugin'
+import { FastifyPlugin, FastifyPluginCallback, FastifyPluginAsync, FastifyPluginOptions } from './plugin'
import { LogLevel } from './logger'
/**
@@ -6,11 +6,21 @@ import { LogLevel } from './logger'
*
* Function for adding a plugin to fastify. The options are inferred from the passed in FastifyPlugin parameter.
*/
-export interface FastifyRegister<T = void> {
+export type FastifyRegister<T = void> = {
<Options extends FastifyPluginOptions>(
plugin: FastifyPlugin<Options>,
opts?: FastifyRegisterOptions<Options>
- ): T;
+ ): T
+} & {
+ <Options extends FastifyPluginOptions>(
+ plugin: FastifyPluginCallback<Options>,
+ opts?: FastifyRegisterOptions<Options>
+ ): T
+} & {
+ <Options extends FastifyPluginOptions>(
+ plugin: FastifyPluginAsync<Options>,
+ opts?: FastifyRegisterOptions<Options>
+ ): T
}
export type FastifyRegisterOptions<Options> = (RegisterOptions & Options) | (() => RegisterOptions & Options)results in test/types/plugin.test-d.ts:43:63
✖ 43:63 Parameter instance implicitly has an any type.
✖ 43:73 Parameter opts implicitly has an any type.
✖ 44:69 Parameter instance implicitly has an any type.
✖ 44:79 Parameter opts implicitly has an any type.
✖ 45:69 Parameter instance implicitly has an any type.
✖ 45:79 Parameter opts implicitly has an any type.
6 errors(where lines 43-45 are uncommented test lines above) If the union is used in test/types/plugin.test-d.ts:26:22
✖ 26:0 Expected an error, but found none.
✖ 26:22 This expression is not callable.
Each member of the union type FastifyRegister<FastifyInstance<Server, IncomingMessage, ServerResponse, FastifyLoggerInstance> & PromiseLike<...>> has signatures, but none of those signatures are compatible with each other.
✖ 27:0 Expected an error, but found none.
✖ 27:22 This expression is not callable.
Each member of the union type FastifyRegister<FastifyInstance<Server, IncomingMessage, ServerResponse, FastifyLoggerInstance> & PromiseLike<...>> has signatures, but none of those signatures are compatible with each other.
✖ 29:44 This expression is not callable.
Each member of the union type FastifyRegister<FastifyInstance<Server, IncomingMessage, ServerResponse, FastifyLoggerInstance> & PromiseLike<...>> has signatures, but none of those signatures are compatible with each other.
✖ 30:44 This expression is not callable.
Each member of the union type FastifyRegister<FastifyInstance<Server, IncomingMessage, ServerResponse, FastifyLoggerInstance> & PromiseLike<...>> has signatures, but none of those signatures are compatible with each other.
✖ 32:44 This expression is not callable.
Each member of the union type FastifyRegister<FastifyInstance<Server, IncomingMessage, ServerResponse, FastifyLoggerInstance> & PromiseLike<...>> has signatures, but none of those signatures are compatible with each other.
✖ 32:63 Parameter instance implicitly has an any type.
...Though since that issue is resolved looks like the types are indeed wrong in some way. I currently don't have time to experiment with this but feel free to just push to this branch if you have more ideas. |
|
@lundibundi I have a commit ready to go can you make sure to enable maintainer access to push to your fork? |
|
Here is the git diff of my solution in case that is easier diff --git a/fastify.d.ts b/fastify.d.ts
index 65c8a5b..26cdfdb 100644
--- a/fastify.d.ts
+++ b/fastify.d.ts
@@ -119,7 +119,7 @@ type TrustProxyFunction = (address: string, hop: number) => boolean
/* Export all additional types */
export { FastifyRequest, RequestGenericInterface } from './types/request'
export { FastifyReply } from './types/reply'
-export { FastifyPlugin, FastifyPluginOptions } from './types/plugin'
+export { FastifyPluginCallback, FastifyPluginAsync, FastifyPluginOptions } from './types/plugin'
export { FastifyInstance } from './types/instance'
export { FastifyLoggerOptions, FastifyLoggerInstance, FastifyLogFn, LogLevel } from './types/logger'
export { FastifyContext } from './types/context'
diff --git a/test/types/plugin.test-d.ts b/test/types/plugin.test-d.ts
index b8c7f04..1cf5de0 100644
--- a/test/types/plugin.test-d.ts
+++ b/test/types/plugin.test-d.ts
@@ -1,27 +1,17 @@
-import fastify, {
- FastifyPlugin,
- FastifyInstance,
- FastifyPluginOptions,
- RawServerBase, RawRequestDefaultExpression, RawReplyDefaultExpression
-} from '../../fastify'
+import fastify, { FastifyInstance, FastifyPluginOptions } from '../../fastify'
import * as http from 'http'
import * as https from 'https'
import { expectType, expectError, expectAssignable } from 'tsd'
-import {FastifyPluginAsync, FastifyPluginCallback} from "../../types/plugin";
+import { FastifyPluginCallback, FastifyPluginAsync } from '../../types/plugin'
// FastifyPlugin & FastifyRegister
interface TestOptions extends FastifyPluginOptions {
option1: string;
option2: boolean;
}
-const testPluginOpts: FastifyPlugin<TestOptions> = function (instance, opts, next) { }
+const testPluginOpts: FastifyPluginCallback<TestOptions> = function (instance, opts, next) { }
-
-// TODO: Remove explicit types when https://github.com/microsoft/TypeScript/issues/32571 is resolved.
-const testPluginOptsAsync: FastifyPlugin<TestOptions> = async function (
- instance: FastifyInstance<RawServerBase, RawRequestDefaultExpression<RawServerBase>, RawReplyDefaultExpression<RawServerBase>>,
- opts: TestOptions
-) { }
+const testPluginOptsAsync: FastifyPluginAsync<TestOptions> = async function (instance, opts) { }
expectError(fastify().register(testPluginOpts, {})) // error because missing required options from generic declaration
expectError(fastify().register(testPluginOptsAsync, {})) // error because missing required options from generic declaration
@@ -39,10 +29,9 @@ expectAssignable<FastifyInstance>(fastify().register(testPluginCallback, {}))
const testPluginAsync: FastifyPluginAsync = async function (instance, opts) { }
expectAssignable<FastifyInstance>(fastify().register(testPluginAsync, {}))
-// TODO: Uncomment when https://github.com/microsoft/TypeScript/issues/32571 is resolved.
-// expectAssignable<FastifyInstance>(fastify().register(function (instance, opts): Promise<void> { }))
-// expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, () => { }))
-// expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, { logLevel: 'info', prefix: 'foobar' }))
+expectAssignable<FastifyInstance>(fastify().register(function (instance, opts): Promise<void> { return Promise.resolve() }))
+expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, () => { }))
+expectAssignable<FastifyInstance>(fastify().register(async function (instance, opts) { }, { logLevel: 'info', prefix: 'foobar' }))
expectError(fastify().register(function (instance, opts, next) { }, { logLevel: '' })) // must use a valid logLevel
diff --git a/types/plugin.d.ts b/types/plugin.d.ts
index c9daa5d..afae888 100644
--- a/types/plugin.d.ts
+++ b/types/plugin.d.ts
@@ -1,30 +1,30 @@
-import { FastifyError } from 'fastify-error'
import { FastifyInstance } from './instance'
import {
RawServerBase,
- RawServerDefault,
RawRequestDefaultExpression,
RawReplyDefaultExpression
} from './utils'
+/**
+ * FastifyPluginCallback
+ *
+ * Fastify allows the user to extend its functionalities with plugins. A plugin can be a set of routes, a server decorator or whatever. To activate plugins, use the `fastify.register()` method.
+ */
export type FastifyPluginCallback<Options extends FastifyPluginOptions = {}> = (
instance: FastifyInstance<RawServerBase, RawRequestDefaultExpression<RawServerBase>, RawReplyDefaultExpression<RawServerBase>>,
opts: Options,
- next: (err?: FastifyError) => void
+ next: (err?: Error) => void
) => void
-export type FastifyPluginAsync<Options extends FastifyPluginOptions = {}> = (
- instance: FastifyInstance<RawServerBase, RawRequestDefaultExpression<RawServerBase>, RawReplyDefaultExpression<RawServerBase>>,
- opts: Options
-) => Promise<void>;
-
/**
- * FastifyPlugin
+ * FastifyPluginAsync
*
* Fastify allows the user to extend its functionalities with plugins. A plugin can be a set of routes, a server decorator or whatever. To activate plugins, use the `fastify.register()` method.
*/
-export type FastifyPlugin<Options extends FastifyPluginOptions = {}> =
- FastifyPluginCallback<Options> | FastifyPluginAsync<Options>
+export type FastifyPluginAsync<Options extends FastifyPluginOptions = {}> = (
+ instance: FastifyInstance<RawServerBase, RawRequestDefaultExpression<RawServerBase>, RawReplyDefaultExpression<RawServerBase>>,
+ opts: Options
+) => Promise<void>;
export interface FastifyPluginOptions {
[key: string]: any;
diff --git a/types/register.d.ts b/types/register.d.ts
index 5eda7bd..d24ca00 100644
--- a/types/register.d.ts
+++ b/types/register.d.ts
@@ -1,4 +1,4 @@
-import { FastifyPlugin, FastifyPluginOptions } from './plugin'
+import { FastifyPluginOptions, FastifyPluginCallback, FastifyPluginAsync } from './plugin'
import { LogLevel } from './logger'
/**
@@ -8,7 +8,15 @@ import { LogLevel } from './logger'
*/
export interface FastifyRegister<T = void> {
<Options extends FastifyPluginOptions>(
- plugin: FastifyPlugin<Options>,
+ plugin: FastifyPluginCallback<Options>,
+ opts?: FastifyRegisterOptions<Options>
+ ): T;
+ <Options extends FastifyPluginOptions>(
+ plugin: FastifyPluginAsync<Options>,
+ opts?: FastifyRegisterOptions<Options>
+ ): T;
+ <Options extends FastifyPluginOptions>(
+ plugin: FastifyPluginCallback<Options> | FastifyPluginAsync<Options>,
opts?: FastifyRegisterOptions<Options>
): T;
} |
|
It should already be enabled Edit: didn't think about overload syntax in |
Ethan-Arrowood
force-pushed
the
fix-plugin-fn-type
branch
from
f154d42
to
488d087
Compare
|
Awesome thank you! Rebased to hopefully fix CI |
|
This seems to be the failing line of the CI: https://github.com/fastify/fastify/pull/2350/checks?check_run_id=831281965#step:6:8030 Not sure why, everything runs locally and I rebased on master |
| next: (err?: FastifyError) => void | ||
| ): void; | ||
| } | ||
| export type FastifyPluginCallback<Options extends FastifyPluginOptions = {}> = ( |
There was a problem hiding this comment.
I think such definition will break libraries such as fastify-autoload which rely upon existence of unified FastifyPlugin type (https://github.com/fastify/fastify-autoload/blob/master/fastify-autoload.d.ts)
I guess they will have to also use overload if there is no other way.
There was a problem hiding this comment.
So all the plugins types would need to be updated again? :/
There was a problem hiding this comment.
We have a few options:
- see if the union or intersection type will satisfy existing plugins
- fix all plugins again (
☹️ ) - add an alias for FastifyPlugin = FastifyPluginCallback as this is what it was before the change; mark as deprecated and then expect that update in v4
There was a problem hiding this comment.
Wouldn't union plugin type work out just fine for plugins, everything will work as it was but it will allow passing explicitly typed async functions to plugins (it just won't support type inference for async functions)?
(with marking this union type deprecated right away)
There was a problem hiding this comment.
I'm not sure off the top of my head but please try it out
|
there are conflicts and tests seems to fail |
|
@lundibundi i know you are low on time so let me know if you'd like for me to continue to complete this pr! Two things need to happen:
|
lundibundi
force-pushed
the
fix-plugin-fn-type
branch
from
488d087
to
b5e3799
Compare
|
Done. Tested the Also tested Can someone recommend other plugins that may be most affected by this change to check? |
|
There is a conflict here. |
Co-authored-by: Ethan Arrowood <ethan.arrowood@gmail.com>
lundibundi
force-pushed
the
fix-plugin-fn-type
branch
from
b5e3799
to
b821e4d
Compare
|
@mcollina done, I thought I rebased 🤷 |
|
@Ethan-Arrowood wdyt? |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Checklist
npm run testandnpm run benchmarkI usually write TS with no-misused-promises rule on since it provides such benefits as prohibiting passing
asyncfunction where it is not expected (i.e. event listener onEventEmitteretc.)The
fastify.registerdidn't have a correct overload for a plugin function that returns a promise therefore that rule failed even though such use-case is supported.This adds an overload to
FastifyPluginthat returns a Promise, therefore, telling TS thatregistercan handle such functions.Unfortunately, I couldn't get TS to properly infer types for async versions of functions, therefore a need to explicitly specify type. Furthermore, since the type was explicitly defined on
FastifyPlugin.instanceTS refused to accept justFastifyInstancetype for instance argument on plugin function and I had to change the signature ofFastifyPluginto accept any extension ofFastifyInstance.Definitely want to hear @Ethan-Arrowood opinion on this.