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.

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

fix: Set FastifyRequest.id type as a string #4992

Merged
merged 1 commit into from Aug 26, 2023
Merged

Conversation

samchungy
Copy link
Contributor

@samchungy samchungy commented Aug 25, 2023

Resolves #4993

@github-actions github-actions bot added the typescript TypeScript related label Aug 25, 2023
@samchungy samchungy changed the title fix: type FastifyRequest id as a string fix: Set FastifyRequest.id type as a string Aug 25, 2023
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

LGTM

@Uzlopak Uzlopak requested a review from a team August 25, 2023 06:45
@climba03003
Copy link
Member

climba03003 commented Aug 25, 2023

ID might not be string since it is the output of the generate id function.

@metcoder95
Copy link
Member

ID might not be string since it is the output of the generate id function.

On types, we somehow make it explicit that the function should return a string (see); although I agree that we do not force this on runtime.
Shall we consider this "valid" accordingly to the types or do we prefer to not do it until we, at least, force coercion to string for the output of the genReqId?

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 25, 2023

No runtime validation/casting please. genReqId option for the fastify constructor defines the function as returning a string. So if somebody provides a custom genReqId which is not returning a string, then this violates our "api contract" already.

@metcoder95
Copy link
Member

No strong opinion about it, so it SGTM 👍

@gurgunday
Copy link
Member

No runtime validation/casting please. genReqId option for the fastify constructor defines the function as returning a string. So if somebody provides a custom genReqId which is not returning a string, then this violates our "api contract" already.

Just wanna +1 this especially when it comes to Fastify since zero overhead is a core principle – that the type definitions are there for a reason and breaking that contract will naturally introduce unexpected behavior, weather it is in the form of errors or something else

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

ID might not be string since it is the output of the generate id function.

Correct, but:

  • by default it is a string
  • if user customizes it, it reads from headers 99% of the times, so it is a string

@Uzlopak Uzlopak merged commit 889456f into fastify:main Aug 26, 2023
28 checks passed
renovate bot added a commit to tomacheese/telcheck that referenced this pull request Aug 27, 2023
[![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.21.0` ->
`4.22.0`](https://renovatebot.com/diffs/npm/fastify/4.21.0/4.22.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify (fastify)</summary>

###
[`v4.22.0`](https://togithub.com/fastify/fastify/releases/tag/v4.22.0)

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.21.0...v4.22.0)

#### What's Changed

- make FastifySchemaValidationError.params wider by
[@&#8203;cm-ayf](https://togithub.com/cm-ayf) in
[fastify/fastify#4476
- docs(ecosystem): add fastify-hashids by
[@&#8203;andersonjoseph](https://togithub.com/andersonjoseph) in
[fastify/fastify#4934
- fix: hasPlugin does not track parent plugins by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4929
- docs: early hints plugin is fastify plugin by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4947
- chore: add pull request title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4951
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.29.6 to 0.30.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4952
- ci: improve pr title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4953
- ci: fix warnings in benchmark workflows by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4954
- docs: fix removeAdditional comment by
[@&#8203;G0maa](https://togithub.com/G0maa) in
[fastify/fastify#4948
- fix: Try to fix parser benchmark workflow by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4956
- fix: infer correct hook handler by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4945
- fix: do not double send the response if the request is destroyed but
not aborted by [@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4963
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.30.4 to 0.31.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4973
- fix: bodyLimit must be applied on fully decoded body by
[@&#8203;sergburn](https://togithub.com/sergburn) in
[fastify/fastify#4969
- chore: updates
[@&#8203;typescript-eslint/eslint-plugin](https://togithub.com/typescript-eslint/eslint-plugin)
and
[@&#8203;typescript-eslint/parser](https://togithub.com/typescript-eslint/parser)
by [@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4977
- chore: use group dependencies in dependabot by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4979
- chore: fix ci bench by [@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4983
- fix: require.cache is undefined breaks SEA by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4982
- ci: remove sync next wf by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4985
- docs: remove mixing ES6 and commomjs in the example by
[@&#8203;thenicolau](https://togithub.com/thenicolau) in
[fastify/fastify#4990
- fix: errorHandler callback should utilize TypeProvider by
[@&#8203;muan](https://togithub.com/muan) in
[fastify/fastify#4989
- types: add onRoute to ApplicationHookLookup by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4968
- chore: make tests pass on ipv4 only machine by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4997
- fix: Set `FastifyRequest.id` type as a string by
[@&#8203;samchungy](https://togithub.com/samchungy) in
[fastify/fastify#4992

#### New Contributors

- [@&#8203;G0maa](https://togithub.com/G0maa) made their first
contribution in
[fastify/fastify#4948
- [@&#8203;thenicolau](https://togithub.com/thenicolau) made their first
contribution in
[fastify/fastify#4990
- [@&#8203;muan](https://togithub.com/muan) made their first
contribution in
[fastify/fastify#4989
- [@&#8203;samchungy](https://togithub.com/samchungy) made their first
contribution in
[fastify/fastify#4992

**Full Changelog**:
fastify/fastify@v4.21.0...v4.22.0

</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.

---

- [ ] <!-- rebase-check -->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).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi41Ni4wIiwidXBkYXRlZEluVmVyIjoiMzYuNTYuMCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Uzlopak added a commit that referenced this pull request Aug 29, 2023
* New plugin rabbitmq (#1)

* Update Ecosystem.md

* added my plugin

* chore: make tests pass on ipv4 only machine (#4997)

Signed-off-by: Matteo Collina <hello@matteocollina.com>

* fix: type FastifyRequest id as a string (#4992)

* Bumped v4.22.0

Signed-off-by: Matteo Collina <hello@matteocollina.com>

---------

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Sam Chung <samchungy@gmail.com>

* fix

---------

Signed-off-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Matteo Collina <hello@matteocollina.com>
Co-authored-by: Sam Chung <samchungy@gmail.com>
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
renovate bot added a commit to redwoodjs/redwood that referenced this pull request Aug 30, 2023
[![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.21.0` ->
`4.22.0`](https://renovatebot.com/diffs/npm/fastify/4.21.0/4.22.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify (fastify)</summary>

###
[`v4.22.0`](https://togithub.com/fastify/fastify/releases/tag/v4.22.0)

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.21.0...v4.22.0)

##### What's Changed

- make FastifySchemaValidationError.params wider by
[@&#8203;cm-ayf](https://togithub.com/cm-ayf) in
[fastify/fastify#4476
- docs(ecosystem): add fastify-hashids by
[@&#8203;andersonjoseph](https://togithub.com/andersonjoseph) in
[fastify/fastify#4934
- fix: hasPlugin does not track parent plugins by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4929
- docs: early hints plugin is fastify plugin by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4947
- chore: add pull request title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4951
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.29.6 to 0.30.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4952
- ci: improve pr title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4953
- ci: fix warnings in benchmark workflows by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4954
- docs: fix removeAdditional comment by
[@&#8203;G0maa](https://togithub.com/G0maa) in
[fastify/fastify#4948
- fix: Try to fix parser benchmark workflow by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4956
- fix: infer correct hook handler by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4945
- fix: do not double send the response if the request is destroyed but
not aborted by [@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4963
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.30.4 to 0.31.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4973
- fix: bodyLimit must be applied on fully decoded body by
[@&#8203;sergburn](https://togithub.com/sergburn) in
[fastify/fastify#4969
- chore: updates
[@&#8203;typescript-eslint/eslint-plugin](https://togithub.com/typescript-eslint/eslint-plugin)
and
[@&#8203;typescript-eslint/parser](https://togithub.com/typescript-eslint/parser)
by [@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4977
- chore: use group dependencies in dependabot by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4979
- chore: fix ci bench by [@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4983
- fix: require.cache is undefined breaks SEA by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4982
- ci: remove sync next wf by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4985
- docs: remove mixing ES6 and commomjs in the example by
[@&#8203;thenicolau](https://togithub.com/thenicolau) in
[fastify/fastify#4990
- fix: errorHandler callback should utilize TypeProvider by
[@&#8203;muan](https://togithub.com/muan) in
[fastify/fastify#4989
- types: add onRoute to ApplicationHookLookup by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4968
- chore: make tests pass on ipv4 only machine by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4997
- fix: Set `FastifyRequest.id` type as a string by
[@&#8203;samchungy](https://togithub.com/samchungy) in
[fastify/fastify#4992

##### New Contributors

- [@&#8203;G0maa](https://togithub.com/G0maa) made their first
contribution in
[fastify/fastify#4948
- [@&#8203;thenicolau](https://togithub.com/thenicolau) made their first
contribution in
[fastify/fastify#4990
- [@&#8203;muan](https://togithub.com/muan) made their first
contribution in
[fastify/fastify#4989
- [@&#8203;samchungy](https://togithub.com/samchungy) made their first
contribution in
[fastify/fastify#4992

**Full Changelog**:
fastify/fastify@v4.21.0...v4.22.0

</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.

---

- [ ] <!-- rebase-check -->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/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42OC4xIiwidXBkYXRlZEluVmVyIjoiMzYuNjguMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
jtoar pushed a commit to redwoodjs/redwood that referenced this pull request Sep 2, 2023
[![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.21.0` ->
`4.22.0`](https://renovatebot.com/diffs/npm/fastify/4.21.0/4.22.0) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/fastify/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/fastify/4.21.0/4.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>fastify/fastify (fastify)</summary>

###
[`v4.22.0`](https://togithub.com/fastify/fastify/releases/tag/v4.22.0)

[Compare
Source](https://togithub.com/fastify/fastify/compare/v4.21.0...v4.22.0)

##### What's Changed

- make FastifySchemaValidationError.params wider by
[@&#8203;cm-ayf](https://togithub.com/cm-ayf) in
[fastify/fastify#4476
- docs(ecosystem): add fastify-hashids by
[@&#8203;andersonjoseph](https://togithub.com/andersonjoseph) in
[fastify/fastify#4934
- fix: hasPlugin does not track parent plugins by
[@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4929
- docs: early hints plugin is fastify plugin by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4947
- chore: add pull request title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4951
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.29.6 to 0.30.2 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4952
- ci: improve pr title check by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4953
- ci: fix warnings in benchmark workflows by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4954
- docs: fix removeAdditional comment by
[@&#8203;G0maa](https://togithub.com/G0maa) in
[fastify/fastify#4948
- fix: Try to fix parser benchmark workflow by
[@&#8203;kibertoad](https://togithub.com/kibertoad) in
[fastify/fastify#4956
- fix: infer correct hook handler by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4945
- fix: do not double send the response if the request is destroyed but
not aborted by [@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4963
- chore: Bump
[@&#8203;sinclair/typebox](https://togithub.com/sinclair/typebox) from
0.30.4 to 0.31.1 by
[@&#8203;dependabot](https://togithub.com/dependabot) in
[fastify/fastify#4973
- fix: bodyLimit must be applied on fully decoded body by
[@&#8203;sergburn](https://togithub.com/sergburn) in
[fastify/fastify#4969
- chore: updates
[@&#8203;typescript-eslint/eslint-plugin](https://togithub.com/typescript-eslint/eslint-plugin)
and
[@&#8203;typescript-eslint/parser](https://togithub.com/typescript-eslint/parser)
by [@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4977
- chore: use group dependencies in dependabot by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4979
- chore: fix ci bench by [@&#8203;Eomm](https://togithub.com/Eomm) in
[fastify/fastify#4983
- fix: require.cache is undefined breaks SEA by
[@&#8203;climba03003](https://togithub.com/climba03003) in
[fastify/fastify#4982
- ci: remove sync next wf by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4985
- docs: remove mixing ES6 and commomjs in the example by
[@&#8203;thenicolau](https://togithub.com/thenicolau) in
[fastify/fastify#4990
- fix: errorHandler callback should utilize TypeProvider by
[@&#8203;muan](https://togithub.com/muan) in
[fastify/fastify#4989
- types: add onRoute to ApplicationHookLookup by
[@&#8203;Uzlopak](https://togithub.com/Uzlopak) in
[fastify/fastify#4968
- chore: make tests pass on ipv4 only machine by
[@&#8203;mcollina](https://togithub.com/mcollina) in
[fastify/fastify#4997
- fix: Set `FastifyRequest.id` type as a string by
[@&#8203;samchungy](https://togithub.com/samchungy) in
[fastify/fastify#4992

##### New Contributors

- [@&#8203;G0maa](https://togithub.com/G0maa) made their first
contribution in
[fastify/fastify#4948
- [@&#8203;thenicolau](https://togithub.com/thenicolau) made their first
contribution in
[fastify/fastify#4990
- [@&#8203;muan](https://togithub.com/muan) made their first
contribution in
[fastify/fastify#4989
- [@&#8203;samchungy](https://togithub.com/samchungy) made their first
contribution in
[fastify/fastify#4992

**Full Changelog**:
fastify/fastify@v4.21.0...v4.22.0

</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.

---

- [ ] <!-- rebase-check -->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/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi42OC4xIiwidXBkYXRlZEluVmVyIjoiMzYuNjguMSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript TypeScript related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tighten FastifyRequest.id type
6 participants