Do you plan to continue @nemanja-tosic?

Do you plan to continue @nemanja-tosic?

Hello. Yes, i am planning on adding plugin dependency type support next hopefully some time next week. I do apologize for radio silence since i created the draft...

Thanks for your work.
It's just to know if you are still motivated as I would like to remove module augmentation from the demo as soon as possible.
If you know a way I can help you, let me know.

Thanks for your work. It's just to know if you are still motivated as I would like to remove module augmentation from the demo as soon as possible. If you know a way I can help you, let me know.

Hi @jean-michelet. I pushed a change that demos the plugin dependency concept. Where i could use a bit of help is first of does it makes sense the way i have it and i could use help with implementation on the fastify side.

It is necessary to pass plugins as object in order to extract types. So far, the same was achieved by passing strings for plugin names, so that will be a change.

The createPlugin utility i have needs to be implemented in fastify-plugin, in a way that makes this interface possible.

I will continue with updating to latest fastify and looking into the fastify-plugin side of things.

Thanks!

Looking into it.

• maybe I'm wrong, but I think you break a lot of stuff right now, I left a couple of comments.

The changes are meant to be backwards compatible to a reasonable degree. There will be some breaking changes, but i'm trying to keep the number down to a minimum. Doing what needs to be done here has certain requirements when it comes to type definitions. I don't see how to be 100% backwards compatible given the fact that previous code could not support what is needed.

• it requires a lot of work to use builder api on existing core plugins like you did, don't know if it's even doable:

The only way i could thing of to gain compile time safety with TypeScript in this scenario is to use return values to calculate the impact of a certain operation. If a method returns a decorated instance, we can infer things from the return type. If a method returns void, there is little that can be done in the context of compile time checks.

It is possible to do plenty in run time, by asserting things on the fastify instance, but that was not the goal of these changes.

Therefore, the switch to utilizing return values is a must the way i see it. Plugins can opt into this by either returning an instance or returning void in which case we cannot infer what the plugin provides, but things are backwards compatible.

It is always possible to create a type definition that specifies what a plugin provides, but unless a plugin returns a value it is not possible to check whether the plugin does what the value says it should do, in compile time.

It is always possible to create a type definition that specifies what a plugin provides

It's probably the more reasonable for now, if you can provide a working example with FastifyStatic, it's great 👍
But I am not opposed on returning decorated instance on core plugins too, I don't have a large enough vision to know if it's achievable.

It's probably the more reasonable for now, if you can provide a working example with FastifyStatic, it's great 👍 But I am not opposed on returning decorated instance on core plugins too, I don't have a large enough vision to know if it's achievable.

Fastify static has a draft PR with the POC. I added the dependencies as well. There are FIXMEs which i am working through.

First point of interest is the declaration of a plugin.

export type FastifyStaticPlugin = UnEncapsulatedPlugin<
  FastifyPluginAsync<
    NonNullable<fastifyStatic.FastifyStaticOptions>,
    AnyFastifyInstance,
    FastifyInstance<any, any, any, any, any, FastifyStaticPluginDecorators>
  >
>

The change is to types only, and the only aim is to provide a valid type definition that contains the decorators. Fastify types should handle properly merging the out instance FastifyInstance<any, any, any, any, any, FastifyStaticPluginDecorators>.

Second point of interest is dependencies

const pluginWithFastifyStaticDependency = createPlugin((instance) =>
  instance.get('/', (req, res) => {
    expectType<FastifyStaticPluginDecorators['reply']['sendFile']>(res.sendFile)
    expectType<FastifyStaticPluginDecorators['reply']['download']>(res.download)
  }), { dependencies: [fastifyStatic] })

expectError(fastify().register(pluginWithFastifyStaticDependency))

fastify().register(fastifyStatic).register(pluginWithFastifyStaticDependency)

The code in fastify is focused on making the calls chainable in a way that provides inference so that you don't have to manually type. Core plugins can be manually annotated, like fastify-static was in this case. When manually annotating the plugin needs to make sure what it returns matches the definition on it's own. When relying on inference, the output must be correct as it's infered.

Let me know what you think!

Let me know what you think!

Looks great!

When do you think your work will be ready?

I hope with some more eyes from the community

I think there is an interest, one of the maintainer shared me your PR, but I don't know if there is a lot of contributors that have advanced knowledges of TypeScript. For me it's a bit hard to read for now and you understand better than me what to do, I think @climba03003 can share a valuable feedback, maybe when your PR will be ready for review.

Anyway, keeping a close eye on it.

Can we support this?

export const deepPluginDependency = createPlugin((instance) =>
  instance
    .decorateReply('deep_dependency_reply', true))

export const pluginDependecy = createPlugin((instance) => instance
  .decorateRequest('dependency_request', true), { dependencies: [deepPluginDependency] })

export const dependantPlugin = createPlugin((instance) => {
  return instance.get('/', (req, res) => {
    expectType<boolean>(req.dependency_request)
    expectType<boolean>(res.deep_dependency_reply)
  })
}, { dependencies: [pluginDependecy] })

I was thinking about this scenario, and i think that we should. I will see what needs to be done to get there.

I had tried in the past and I remember the compiler thrown this error:

Type instantiation is excessively deep and possibly infinite.

I think it's possible to ignore it or set an arbitrary depth level.

Added support for transitive dependencies.

@jean-michelet could you give me a hand with completing the fastify-plugin side of things please?

At this moment i have the following definition which i used throughout the unit tests which currently passes all the tests.

export function createPlugin<
  TPlugin extends FastifyPluginCallback,
  TDependencies extends FastifyDependencies,
  TEnhanced extends ApplyDependencies<TPlugin, TDependencies> = ApplyDependencies<TPlugin, TDependencies>
> (plugin: TEnhanced, options?: { dependencies?: TDependencies }): UnEncapsulatedPlugin<TEnhanced>
export function createPlugin<
  TPlugin extends FastifyPluginCallback,
  TDependencies extends FastifyDependencies,
  TEnhanced extends ApplyDependencies<TPlugin, TDependencies> = ApplyDependencies<TPlugin, TDependencies>
> (plugin: TEnhanced, options?: { dependencies?: TDependencies }): UnEncapsulatedPlugin<TEnhanced>
export function createPlugin<
  TPlugin extends FastifyPluginAsync,
  TDependencies extends FastifyDependencies,
  TEnhanced extends ApplyDependencies<TPlugin, TDependencies> = ApplyDependencies<TPlugin, TDependencies>
> (plugin: TEnhanced, options?: { dependencies?: TDependencies }): UnEncapsulatedPlugin<TEnhanced>
export function createPlugin<
  TPlugin extends FastifyPlugin,
  TDependencies extends FastifyDependencies,
  TEnhanced extends ApplyDependencies<TPlugin, TDependencies> = ApplyDependencies<TPlugin, TDependencies>
> (plugin: TEnhanced, options?: { dependencies?: TDependencies }): UnEncapsulatedPlugin<TEnhanced> {
  return plugin as UnEncapsulatedPlugin<TEnhanced>
}

I could use a hand on the implementation side of things.

I am also quite torn on whether this should be provided alongside fastify core, because i needed it most of the time during testing. Do you have an opinion on that?

Thanks!

I didn't see that fastify-plugin was a dev dependency of fastify. I took advantage of that and create a new createPlugin function in fastify-plugin. We can either use a new function or change the old one. I'll weigh the options. I believe unless anyone can come up with further things we can tests i've reached a point where i'd say it could be reviewed.

What remains then is to figure out the final implementation details in fastify-plugin.

I'll take a look this week, thanks again for your work!

Plz @climba03003, can you take a look took too, it looks like an interesting PR.

This update contains important breaking changes. For instance, the number (and type) of the generics for the following types are updated:

1. FastifyPluginCallback
2. FastifyPluginAsync
3. FastifyPlugin
4. FastifyReply (Decorators generic is added at the current position of ReplyType generic)
5. FastifyRequest (as above)

If we want to proceed that way, I think we should mark it as a breaking change (so it should be included in v 6.x). Anyway, if possible, it'd be better to update this code so that we can keep TypeScript compatibility in the current version, so that we can ship it as it is.

There is a comment for FastifyRequest for RequestType, i assumed the same applies for FastifyReply. Granted the same comment isn't present, but it looks to be the same case.

  Decorators extends FastifyDecorators['request'] = FastifyDecorators['request'],
  RequestType extends FastifyRequestType = ResolveFastifyRequestType<TypeProvider, SchemaCompiler, RouteGeneric>
// ^ Temporary Note: RequestType has been re-ordered to be the last argument in
//   generic list. This generic argument is now considered optional as it can be
//   automatically inferred from the SchemaCompiler, RouteGeneric and TypeProvider
//   arguments. Implementations that already pass this argument can either omit
//   the RequestType (preferred) or swap Logger and RequestType arguments when
//   creating custom types of FastifyRequest. Related issue #4123

EDIT: that said, decorators CAN be last, since i don't expect the to be explicitly passed. I only did it this way to preserve what i thought was wanted.

I think we can assume we'll ship this in v6 unless it could be made less breaking. This will give us time to go update all the plugins too.

@nemanja-tosic could you add a little bit of a migration guide? Thanks

I'm not seeing the comments i have on a reviewed file so i will summarize here. The concern for PluginCallback is valid due to the type change, since the type is used to declare plugins and forcing a migration immediately might be difficult. I will take a look if it is possible to support the previous version alongside the new one using https://github.com/fastify/fastify-type-provider-json-schema-to-ts as an example. Acceptance criteria would be that

• fastify-static is changed so that module augmentation is removed but decorators are added (done)
• fastify-type-provider-json-schema-to-ts can remain unchanged (in progress)

Based on that i will know what is needed for migration.

Based on the reviews, FastifyPluginCallback, FastifyPluginCallbackAsync and FastifyPlugin are the main concerns for the work so far, so i will attempt to make those backwards compatible. Thank you for the review @rozzilla

Acceptance criteria would be that

• fastify-static is changed so that module augmentation is removed but decorators are added (done)
• fastify-type-provider-json-schema-to-ts can remain unchanged (in progress)

Yup, exactly! By moving Decorators as the last param (and with a default value) the change will be backward compatible.

I would also add another criteria we should consider:

• existing tsd tests for types, should remain as of today

If we need to update an existing type test, this means the change is not backwards compatible

tsd cannot remain as they are today due to the differences between expectType and expectAssignable. The changes update the instance type in order to enforce type safety, so tests will likely have to follow suite.

@jean-michelet could you take another look at this PR?

I don't feel confident/competent enough to give more feedback than I've already given.

In short :

1. I am a very pessimistic person.
   Even if we don't really understand how we could break the ecosystem for now, I think these changes should not be accepted without first consulting package maintainers that rely heavily on Fastify generics (or at least ping them for a review, so they can choose to ignore).

2. I think the changes lead to a major improvement in developer experience, so some breaking changes are acceptable to me. Once we have more positive feedback, you can ping me and I help you to update the whole core plugin ecosystem. We can also create a new branch on the demo as a showcase for migration.

Closing as too old