docs(decorators): fix TypeScript inconsistency#6224
Conversation
- Remove TypeScript generic syntax from getDecorator and setDecorator sections - Convert all TypeScript examples to JavaScript for consistency - Simplify complex TypeScript-specific concepts - Maintain JavaScript style used throughout the rest of the document Fixes fastify#6222
|
Thanks for your PR.
No, you've just deleted everything, there's an open issue to discusses what to do. |
jsumners
left a comment
There was a problem hiding this comment.
Looking good to me. But I wonder if there is information being lost that should be present in the actual TypeScript documentation -- https://fastify.dev/docs/latest/Reference/TypeScript/
- Move TypeScript-specific decorator content to TypeScript.md instead of deleting - Keep JavaScript-focused documentation in Decorators.md consistent with rest of document - Add comprehensive TypeScript decorators section with advanced typing examples - Preserve valuable information while addressing the original inconsistency issue Addresses feedback from jean-michelet and jsumners on PR fastify#6224
|
@jean-michelet @jsumners Thank you for the feedback! I've completely revised my approach based on your comments: Changes Made:
Result:
This addresses both the original issue (#6222) and the concerns raised in the review. |
|
There are several overlapping concepts related to the use of getDecorator. This isn't strictly TypeScript or JavaScript, but rather a mix of both, along with architectural patterns for working with Fastify. I understand your concern about preserving the documentation reference, and I’m fine with a PR conforming to the goal of Reference. However, I’d really appreciate it if we could find a place to explain what the original documentation was proposing. Is there a place for a new Guide ? |
Fdawgs
left a comment
There was a problem hiding this comment.
Blocking until I get the time to review.
jsumners
left a comment
There was a problem hiding this comment.
Looks good to me, just needs a linting pass.
Yes. |
jean-michelet
left a comment
There was a problem hiding this comment.
Perhaps the Typescript document should be more like the Reference document, but for types?
The different ways to structure an application with getDecorator could be part of a Guide.
These are just suggestions, I am not blocking anything and let the other collaborators decide.
- Fix line length violations in docs/Reference/Decorators.md - Fix line length violations in docs/Reference/TypeScript.md - All lines now comply with 80 character limit - Addresses linting feedback from PR review
|
Hey @jsumners linting issue should be solved now |
jean-michelet
left a comment
There was a problem hiding this comment.
I think I should push a commit to make the changes by myself and gather your feedback.
- Remove 'Plugin encapsulation' bullet point from Decorators.md - Consolidate overlapping bullet points into single 'Dependency validation' point - Move TypeScript Decorators section under Plugins section as requested - Change heading level from ### to #### for proper nesting Addresses feedback from PR review comments
|
Thanks for the feedback @jean-michelet! I've addressed all your suggestions |
…ge from TypeScript decorators section - Change **Note**: format to ℹ Note: format - Remove redundant 'Use Cases' sections and consolidate content - Addresses feedback from PR review comments
|
Thanks for the detailed feedback, I've addressed all the requested changes |
Signed-off-by: Frazer Smith <frazer.dev@icloud.com>
jean-michelet
left a comment
There was a problem hiding this comment.
The chapter already mention the typing of decorators, getDecorator is just an other way to work without module augmentation.
Once this point addressed, it will be good to me.
gurgunday
left a comment
There was a problem hiding this comment.
lgtm except @jean-michelet's comment
Refactor the documentation for Fastify's `getDecorator<T>` and `setDecorator<T>` methods to emphasize their support for generic type parameters, improving type safety and TypeScript integration. This change clarifies the usage of decorators in Fastify and links to the relevant Decorators reference.
|
everything should be ok now! |
jean-michelet
left a comment
There was a problem hiding this comment.
I am gonna unsubscribe to this PR notifications, will eventually push a guide.
@jsumners good for me when you want to merge it.
- Simplify setDecorator description in TypeScript.md - Simplify getDecorator description in TypeScript.md - Streamline setDecorator note in Decorators.md Resolves all unresolved review comments to unblock PR merge.
|
Thanks for the feedbacks. I think it's ready to go |
|
Thanks @emicovi, could you sort the linting out please and then this is good to merge. :) |
|
@emicovi Linting still needs sorting as I said a month ago. |
|
Allrighty... |
Description
Fixes the TypeScript syntax inconsistency in the Decorators documentation as reported in #6222.
Changes Made
docs/Reference/TypeScript.mdinstead of deleting itFiles Changed
docs/Reference/Decorators.md: Updated to JavaScript-focused documentationdocs/Reference/TypeScript.md: Added new "Decorators" section with TypeScript-specific contentIssue Reference
Fixes #6222
Addresses Feedback
Testing