Overload `DecorationMethod` to fix #4870 by voxpelli · Pull Request #4874 · fastify/fastify

voxpelli · 2023-07-03T10:02:42Z

Overloads DecorationMethod to fix #4870.

Adds two overloads:

Plain app.decorateRequest('something') as suggested by @mcollina in Example of decorateRequest can't pass type checking #4870 (comment)
An app.decorateRequest('something', null, ['something-else']) where undefined and null are both accepted. Confirmed needed in Example of decorateRequest can't pass type checking #4870 (comment)

Notably is:

app.decorateRequest('something', null) is still not allowed if something is typed to not allow null

I think this is a good compromise? Thoughts @climba03003, @trim21?

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

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

mcollina

This does not include a test for decorateRequest and decorateReply

voxpelli · 2023-07-03T10:07:41Z

This does not include a test for decorateRequest and decorateReply

I can add that, I didn't as they all use DecorationMethod and as such are indirectly tested

voxpelli · 2023-07-03T10:10:42Z

Should I add typed tests for decorateRequest and decorateReply as well?

mcollina · 2023-07-03T10:11:07Z

yes, thanks!

voxpelli · 2023-07-03T10:17:03Z

@mcollina Done 🙏

climba03003

Can you loosen to accept decorate('name', null) for now and provide strict version in next branch?

I believe it impact the most people if we choose to restrict it in minor or patch release.

voxpelli · 2023-07-03T10:46:21Z

@climba03003 Done ✔️

I wanted to do:

  /** @deprecated */
  (property: string | symbol, value: null): Return;

But then VSCode also showed the method as being deprecated whenever no overload was matching, and I think that would create more confusion than it would help:

voxpelli · 2023-07-03T10:47:00Z

How do I provide strict version in next branch? Another PR after this one has been merged?

climba03003 · 2023-07-03T11:02:28Z

Another PR after this one has been merged?

After we merge the main branch to next, submit PR based on next branch.

climba03003 · 2023-07-03T12:50:06Z

next branch is in sync now.

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [fastify](https://www.fastify.io/) ([source](https://togithub.com/fastify/fastify)) | [`4.19.1` -> `4.19.2`](https://renovatebot.com/diffs/npm/fastify/4.19.1/4.19.2) | [![age](https://badges.renovateapi.com/packages/npm/fastify/4.19.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/fastify/4.19.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/fastify/4.19.2/compatibility-slim/4.19.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/fastify/4.19.2/confidence-slim/4.19.1)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>fastify/fastify (fastify)</summary> ### [`v4.19.2`](https://togithub.com/fastify/fastify/releases/tag/v4.19.2) [Compare Source](https://togithub.com/fastify/fastify/compare/v4.19.1...v4.19.2) #### What's Changed - fix(typescript): route config should not pass url and method by [@climba03003](https://togithub.com/climba03003) in [https://github.com/fastify/fastify/pull/4872](https://togithub.com/fastify/fastify/pull/4872) - Overload `DecorationMethod` to fix [#4870](https://togithub.com/fastify/fastify/issues/4870) by [@voxpelli](https://togithub.com/voxpelli) in [https://github.com/fastify/fastify/pull/4874](https://togithub.com/fastify/fastify/pull/4874) - fix: Type inference breaking when Reply generic is not passed by [@aadito123](https://togithub.com/aadito123) in [https://github.com/fastify/fastify/pull/4875](https://togithub.com/fastify/fastify/pull/4875) - fix: regression with close and serverFactory by [@mcollina](https://togithub.com/mcollina) in [https://github.com/fastify/fastify/pull/4876](https://togithub.com/fastify/fastify/pull/4876) **Full Changelog**: fastify/fastify@v4.19.1...v4.19.2 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/tomacheese/telcheck).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

voxpelli · 2023-07-03T21:54:17Z

Filed an issue in TS to try and make a @deprecate feasible in cases like these in the future: microsoft/TypeScript#54872 (Way easier to make a PR to fastify than to checker.ts in typescript 😬)

Preferred to use `decorate('name')` , `decorateRequest('name')`, `decorateReply('name')` variants instead Deprecation not feasible because of microsoft/TypeScript#54872 Follow up to fastify#4874

voxpelli · 2023-07-04T11:40:22Z

next branch is in sync now.

@climba03003 Opened: #4878

* Disallow `decorate('name', null)` in the types Preferred to use `decorate('name')` , `decorateRequest('name')`, `decorateReply('name')` variants instead Deprecation not feasible because of microsoft/TypeScript#54872 Follow up to #4874 * Docs: Drop `null` from placeholder decorations Signed-off-by: Pelle Wessman <pelle@kodfabrik.se> --------- Signed-off-by: Pelle Wessman <pelle@kodfabrik.se>

github-actions · 2024-07-05T00:23:07Z

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.

Overload DecorationMethod to fix fastify#4870

f728526

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

github-actions bot added the typescript TypeScript related label Jul 3, 2023

mcollina reviewed Jul 3, 2023

View reviewed changes

Add additional tests for other decorators

646900f

voxpelli requested a review from mcollina July 3, 2023 10:10

Test all of DecorationMethod on all decorators

cd83171

climba03003 reviewed Jul 3, 2023

View reviewed changes

Allow decorate('name', null) for now

0e5ff38

climba03003 approved these changes Jul 3, 2023

View reviewed changes

mcollina merged commit 956bc05 into fastify:main Jul 3, 2023

voxpelli mentioned this pull request Jul 3, 2023

Have createUnionOfSignaturesForOverloadFailure() combine @deprecated tags properly microsoft/TypeScript#54872

Closed

voxpelli mentioned this pull request Jul 4, 2023

Next: Disallow decorate('name', null) in the types #4878

Merged

4 tasks

voxpelli deleted the voxpelli/issue-4870 branch July 4, 2023 11:40

renovate bot mentioned this pull request Sep 15, 2023

chore(deps): update all non-major dependencies dan-online/luna#26

Open

1 task

github-actions bot locked as resolved and limited conversation to collaborators Jul 5, 2024

Uh oh!

Conversation

voxpelli commented Jul 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

voxpelli commented Jul 3, 2023

Uh oh!

voxpelli commented Jul 3, 2023

Uh oh!

mcollina commented Jul 3, 2023

Uh oh!

voxpelli commented Jul 3, 2023

Uh oh!

climba03003 left a comment

Choose a reason for hiding this comment

Uh oh!

voxpelli commented Jul 3, 2023

Uh oh!

voxpelli commented Jul 3, 2023

Uh oh!

climba03003 commented Jul 3, 2023

Uh oh!

climba03003 commented Jul 3, 2023

Uh oh!

voxpelli commented Jul 3, 2023

Uh oh!

voxpelli commented Jul 4, 2023

Uh oh!

github-actions bot commented Jul 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

voxpelli commented Jul 3, 2023 •

edited

Loading