Conversation
jsumners
left a comment
There was a problem hiding this comment.
Does this mean any plugin a TS user loads will have to have TS types exported?
|
@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 |
mcollina
left a comment
There was a problem hiding this comment.
I'm -1 for this change, it's too breaking.
jsumners
left a comment
There was a problem hiding this comment.
👎
Plugins cannot be required to be TS specific.
|
@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? |
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
left a comment
There was a problem hiding this comment.
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.
|
Added some tests and a mention in TS docs about decorate typechecking |
airhorns
left a comment
There was a problem hiding this comment.
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.
test/types/instance.test-d.ts
Outdated
| expectError(server.decorate('functionWithTypeDefinition', (foo: any, bar: any) => {})) // error because invalid return type | ||
| expectError(server.decorate('functionWithTypeDefinition', (foo: any, bar: any) => true)) // error because doesn't return a promise | ||
| expectError(server.decorate('functionWithTypeDefinition', async (foo: any, bar: any, qwe: any) => true)) // error because too many args | ||
| server.decorate('functionWithTypeDefinition', async (foo, bar) => true) |
There was a problem hiding this comment.
Could you add a type assertion on the type of foo inside this function? Would be good to test that it is inferred properly
|
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. |
Co-authored-by: Ethan Arrowood <ethan@arrowood.dev>
delvedor
left a comment
There was a problem hiding this comment.
I like this change, and I agree it should be treated as semver-major.
|
@Ethan-Arrowood could you take a final look? |
|
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. |
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 anyhacks. Looking for comments about this approachChecklist
npm run testandnpm run benchmarkand the Code of conduct