Skip to content

fix: handle missing route options#236

Merged
gurgunday merged 6 commits intofastify:v8.xfrom
c0sx:handle-missing-route-options
Sep 24, 2024
Merged

fix: handle missing route options#236
gurgunday merged 6 commits intofastify:v8.xfrom
c0sx:handle-missing-route-options

Conversation

@c0sx
Copy link

@c0sx c0sx commented Sep 21, 2024

Fixes #232

in fastify < 4.10 req in onRequest hook didn't have routeOptions at all - PR
in fastify < 4.23 routeOptions didn't have config property - PR

I handled both cases to keep compatibility, but I don't know how add tests for it. Do you have any suggestions?

Checklist

@c0sx c0sx marked this pull request as draft September 21, 2024 17:26
@c0sx c0sx marked this pull request as ready for review September 21, 2024 17:43
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 unable to reproduce the bug from the repository you linked in the issue.
You should first write a test that fails in order to prove the existence of the bug, then write the fix that make it pass.

@c0sx
Copy link
Author

c0sx commented Sep 22, 2024

I am unable to reproduce the bug from the repository you linked in the issue. You should first write a test that fails in order to prove the existence of the bug, then write the fix that make it pass.

Please check fastify version. It should be 4.0.0-4.22.0 to reproduce the bug. I can't write tests because of it.

@c0sx c0sx requested a review from jean-michelet September 22, 2024 06:54
@jean-michelet jean-michelet requested a review from Eomm September 22, 2024 07:45
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.

LGTM

@jean-michelet
Copy link
Member

I think it is possible to run versioned tests in a child process, but I am not sure that this is something we usually do.

@c0sx
Copy link
Author

c0sx commented Sep 22, 2024

I think it is possible to run versioned tests in a child process, but I am not sure that this is something we usually do.

It could be useful, but I'm not sure too.

Usually I'm doing this with docker-compose, but I it requires changes in CI pipeline.

@gurgunday
Copy link
Member

Yeah #228 was breaking for v8 (Fastify v4 line) actually, it should have considered the then-deprecated setting as well instead of just changing to routeOptions

@jean-michelet
Copy link
Member

Can we merge this?

@gurgunday gurgunday merged commit 96830fd into fastify:v8.x Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments