Skip to content

docs: fix TypeScript inconsistency in Decorators.md #6224

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
Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

docs: fix TypeScript inconsistency in Decorators.md #6224

wants to merge 14 commits into from

Conversation

emicovi
Copy link
Contributor

@emicovi emicovi commented Jun 18, 2025 •
edited
Loading

Description

Fixes the TypeScript syntax inconsistency in the Decorators documentation as reported in #6222.

Changes Made

  • Moved TypeScript-specific content to docs/Reference/TypeScript.md instead of deleting it
  • Updated Decorators.md to use consistent JavaScript style matching other decorator methods
  • Added comprehensive TypeScript decorators section with advanced typing examples
  • Preserved all valuable information while addressing the original inconsistency issue

Files Changed

  • docs/Reference/Decorators.md: Updated to JavaScript-focused documentation
  • docs/Reference/TypeScript.md: Added new "Decorators" section with TypeScript-specific content

Issue Reference

Fixes #6222

Addresses Feedback

  • Addresses @jean-michelet's concern about deleting valuable content
  • Addresses @jsumners's suggestion to move TypeScript info to TypeScript documentation
  • Maintains JavaScript consistency as requested in the original issue

Testing

  • Documentation builds without errors
  • All code examples are valid JavaScript/TypeScript
  • Consistent style with existing decorator methods
  • Cross-references work correctly

@emicovi
docs: fix TypeScript inconsistency in Decorators.md
50fd80e 
- 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
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jun 18, 2025
@jean-michelet
Copy link
Member

jean-michelet commented Jun 18, 2025 •
edited
Loading

Thanks for your PR.

  • Simplified complex TypeScript-specific concepts (module augmentation, OmitThisParameter)
  • Converted all TypeScript code examples to JavaScript for consistency
  • Replaced TypeScript-specific sections with JavaScript-focused alternatives

No, you've just deleted everything, there's an open issue to discusses what to do.
If the author hasn't made a PR directly, it's probably because he'd like some feedback before doing so.
I suggest to to wait before merging this.

jsumners
jsumners reviewed Jun 18, 2025
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/

@jsumners jsumners requested review from Fdawgs and jean-michelet June 18, 2025 11:09
@emicovi
docs: revise approach based on maintainer feedback
dbd1d8a 
- 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
@emicovi
Copy link
Contributor Author

emicovi commented Jun 18, 2025

@jean-michelet @jsumners Thank you for the feedback! I've completely revised my approach based on your comments:

Changes Made:

  • Moved TypeScript content to proper location: Added a comprehensive "Decorators" section to docs/Reference/TypeScript.md instead of deleting the valuable information
  • Preserved all content: Nothing was deleted - all TypeScript-specific examples, module augmentation alternatives, and advanced typing features are now properly organized in the TypeScript documentation
  • Maintained JavaScript consistency: Updated docs/Reference/Decorators.md to use consistent JavaScript style matching the rest of the document
  • Added cross-references: JavaScript docs now point TypeScript users to the appropriate TypeScript documentation section

Result:

  • JavaScript users get clean, consistent documentation without confusing TypeScript syntax
  • TypeScript users get comprehensive advanced examples in the proper location
  • No valuable information was lost - it's now better organized

This addresses both the original issue (#6222) and the concerns raised in the review.

@jean-michelet
Copy link
Member

@jsumners

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 ?
I think it can be of interest, I had positive reactions on LinkedIn regarding this way of working.

Fdawgs
Fdawgs requested changes Jun 19, 2025
View reviewed changes
Copy link
Member

@Fdawgs Fdawgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking until I get the time to review.

jsumners
jsumners reviewed Jun 19, 2025
View reviewed changes
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just needs a linting pass.

@jsumners
Copy link
Member

Is there a place for a new Guide ?

Yes.

jean-michelet
jean-michelet reviewed Jun 19, 2025
View reviewed changes
Copy link
Member

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@emicovi
fix: resolve markdown linting issues
a66ca6d 
- 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
@emicovi
Copy link
Contributor Author

emicovi commented Jun 19, 2025

Hey @jsumners linting issue should be solved now

@jsumners jsumners requested a review from a team June 19, 2025 12:38
jean-michelet
jean-michelet reviewed Jun 23, 2025
View reviewed changes
Copy link
Member

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should push a commit to make the changes by myself and gather your feedback.

@emicovi
fix: address PR feedback from jean-michelet
be3270d 
- 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
@emicovi
Copy link
Contributor Author

emicovi commented Jun 23, 2025

Thanks for the feedback @jean-michelet! I've addressed all your suggestions

Fdawgs
Fdawgs reviewed Jun 23, 2025
View reviewed changes
Fdawgs
Fdawgs reviewed Jun 23, 2025
View reviewed changes
Fdawgs
Fdawgs reviewed Jun 23, 2025
View reviewed changes
Fdawgs
Fdawgs reviewed Jun 23, 2025
View reviewed changes
Fdawgs
Fdawgs reviewed Jun 23, 2025
View reviewed changes
@emicovi
fix: address PR feedback from Fdawgs - Remove second-person 'you' usa…
4c6776a 
…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
@emicovi
Copy link
Contributor Author

emicovi commented Jun 24, 2025

Thanks for the detailed feedback, I've addressed all the requested changes

Fdawgs
Fdawgs reviewed Jun 24, 2025
View reviewed changes
@Fdawgs
Update docs/Reference/TypeScript.md
de66e2a 
Signed-off-by: Frazer Smith <frazer.dev@icloud.com>
jean-michelet
jean-michelet reviewed Jun 28, 2025
View reviewed changes
Copy link
Member

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
gurgunday approved these changes Jul 10, 2025
View reviewed changes
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm except @jean-michelet's comment

emicovi added 2 commits August 6, 2025 16:27
@emicovi
docs: update TypeScript decorators section for enhanced type safety
b5a1825 
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.
@emicovi
Merge branch 'main' into fix-decorators-typescript-inconsistency
219ac49
@emicovi
Copy link
Contributor Author

emicovi commented Aug 6, 2025

everything should be ok now!

jean-michelet
jean-michelet reviewed Aug 6, 2025
View reviewed changes
Copy link
Member

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am gonna unsubscribe to this PR notifications, will eventually push a guide.
@jsumners good for me when you want to merge it.

@emicovi
fix: address final review feedback from jean-michelet
026d6cb 
- 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.
@emicovi emicovi requested a review from jsumners August 6, 2025 15:28
@emicovi
Copy link
Contributor Author

emicovi commented Aug 6, 2025

Thanks for the feedbacks. I think it's ready to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jean-michelet jean-michelet jean-michelet left review comments

@Fdawgs Fdawgs Fdawgs approved these changes

@gurgunday gurgunday gurgunday approved these changes

@jsumners jsumners Awaiting requested review from jsumners

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Decorators document suddenly has non-JavaScript
5 participants
@emicovi @jean-michelet @jsumners @Fdawgs @gurgunday