Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Figure out what to do with typed decorators #4110

Closed
2 tasks done
wyozi opened this issue Jul 4, 2022 · 2 comments · Fixed by #4111
Closed
2 tasks done

Figure out what to do with typed decorators #4110

wyozi opened this issue Jul 4, 2022 · 2 comments · Fixed by #4111

Comments

@wyozi
Copy link
Contributor

wyozi commented Jul 4, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

When this PR was merged to main in this commit, it also removed all the changes made in this PR

Ideally AFAIC PR 2981 would be re-added. This might be a bit problematic since #3203 hijacked the first generic slot for all decorate* functions, so re-adding it would be a breaking change. Also this means that since typed decorators wasn't actually added in v4, the feature isn't and cannot be used by anyone.

There's few options I can think of:

  1. Re-add typed decorators as a supplement to 'this types' even if it's a (very small) breaking change
  2. Add typed decorators to decorate* functions in fastify v5
  3. Introduce typedDecorate* functions that do the same as decorate* but with type checking
  4. Forget that typed decorators can even exist and ignore this issue

There's also this tiny documentation change that should probably be removed now: https://github.com/fastify/fastify/pull/2981/files#diff-c2db295d2a99e431bb24ac08c3953353dbb702f62b48587aa2cd76a0b78e0dbf
wyozi referenced this issue Jul 4, 2022
@mcollina
Merge branch 'main' into next
d37d899
@mcollina
Copy link
Member

mcollina commented Jul 4, 2022

Would you like to send a PR for restoring the functionality that was added in 2981 and I later removed by mistake? I would prefer to not wait for v5... and also that was slotted in to be released in v4.

@wyozi
Copy link
Contributor Author

wyozi commented Jul 4, 2022

Made #4111 with something that could at least be a starting point

@wyozi wyozi mentioned this issue Jul 4, 2022
Merged
4 tasks
voxpelli added a commit to voxpelli/fastify that referenced this issue Jun 26, 2023
@voxpelli
Extend decorators to enforce function returns
8281091 
Non-breaking backwards compatible enhancement of decorators.

Should not have the issues of fastify#4110 which was reverted in fastify#4129
voxpelli added a commit to voxpelli/fastify that referenced this issue Jun 26, 2023
@voxpelli
feat: have decorate() etc enforce typed values
dcda004 
Non-breaking backwards compatible enhancement of decorators.

Should not have the issues of fastify#4110 which was reverted in fastify#4129

Signed-off-by: Pelle Wessman <pelle@kodfabrik.se>
mcollina pushed a commit that referenced this issue Jun 30, 2023
@voxpelli
Extend decorate() etc to enforce defined values (#4858)
1b393ad 
* feat: have `decorate()` etc enforce typed values

Non-breaking backwards compatible enhancement of decorators.

Should not have the issues of #4110 which was reverted in #4129

Signed-off-by: Pelle Wessman <pelle@kodfabrik.se>

* test: test typed decoration properties

* Add support + tests for `getter`/`setter` interface

* Use the same definition for all decoration methods

---------

Signed-off-by: Pelle Wessman <pelle@kodfabrik.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Re-add typed decorators wyozi/fastify
2 participants
@mcollina @wyozi