Typed decorators

[wyozi]

Again a quite strong type change, but I think an important one as well for type safety. This would mean that all decorators must be explicitly typed in the interface to go through without as any hacks. Looking for comments about this approach

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[jsumners]

Does this mean any plugin a TS user loads will have to have TS types exported?

[wyozi]

@jsumners ah yeah, this change would mean that plugins that don't export types in the correct format would start causing TS compiler errors. That's not ideal.

Alternative that I'm doing in my codebase right now is a separate typedDecorate function on FastifyInstance. It's slightly annoying to have two functions for the same thing, but I'm trying to have as strict typing as possible everywhere I can.

[mcollina]

I'm -1 for this change, it's too breaking.

[jsumners]

👎

Plugins cannot be required to be TS specific.

[wyozi]

@mcollina @jsumners I thought of a better way to make this work.

Now there's two type declarations for decorate: one for when the key exists in the interface type and fallback for when the key doesn't exist. This way the stricter typechecking will only kick in when the decorator key is found in the real type interface. It's not perfect but it shouldn't break anything while still allowing stricter type checks when possible.

What do you think?

[jsumners]

one for when the key exists in the interface type and fallback for when the key doesn't exist.

I don't know what this means. But if it doesn't break JavaScript plugins, then I am okay with whatever @fastify/typescript decides.

[Ethan-Arrowood]

can you add some type definition tests to demonstrate how this is meant to work? Additionally, I think you'll need to make some doc updates.

[wyozi]

Added some tests and a mention in TS docs about decorate typechecking

[airhorns]

I like this approach! I think it is a good balance between allowing explicitness if folks want to opt in without requiring it, which is the same aesthetic as TypeScript itself seems to use.

AFAICT this is still a breaking change in that projects might no longer typecheck if their declarations don't match their decorations. But, that is a real problem if you ask me, and worth identifying. Dunno if that qualifies as semver major but we should definitely add a note to the change log and instructions for remedying the issue.

[airhorns]

Could you add a type assertion on the type of foo inside this function? Would be good to test that it is inferred properly

[mcollina]

i would prefer if this targeted the next branch instead

[jsumners]

i would prefer if this targeted the next branch instead

Even then, I would be open to a rollback if it breaks the overall ecosystem.

[mcollina]

lgtm

.

[delvedor]

I like this change, and I agree it should be treated as semver-major.

[mcollina]

@Ethan-Arrowood could you take a final look?

[RafaelGSS]

LGTM.

[Ethan-Arrowood]

LGTM 😄

[github-actions]

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.