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 (types): this is FastifyInstance #3622

Merged
merged 9 commits into from Jan 17, 2022
Merged

fix (types): this is FastifyInstance #3622

merged 9 commits into from Jan 17, 2022

Conversation

darkgl0w
Copy link
Member

@darkgl0w darkgl0w commented Jan 15, 2022

Hello.

This PR aims to :

Checklist

@github-actions github-actions bot added the typescript TypeScript related label Jan 15, 2022
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

Can you add an unit test for this changes? Not compulsory, but better to prevent the this problem in future.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

can you please add an assertion so we don't regress?

@darkgl0w
Copy link
Member Author

Yeah I will be home in maybe 10/15 minutes.
I will add a proper use case to prevent any future regression on this.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

@climba03003 could you take a final look? I plan to ship this tomorrow.

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

LGTM.

@darkgl0w
Copy link
Member Author

darkgl0w commented Jan 16, 2022

CI fails on what appears to be an unrelated flacky test :d

Hmmm, ok at least 2 flacky tests spotted:

  • the 1st one is in the 404s.test.js file at line 1224. IMO it can be fixed by passing the tap timeout option :
- t.test('preHandler hook in setNotFoundHandler should be called when callNotFound', t => {
+ t.test('preHandler hook in setNotFoundHandler should be called when callNotFound', { timeout: 40000 }, t => {
  • the 2nd one is in the custom-parser.test.js file at line 1381.
    It returns a 503 HTTP status code (Service Unavailable) instead of the 200 expected one. I will try to see if it can be fixed :d

@github-actions github-actions bot removed the typescript TypeScript related label Jan 16, 2022
@darkgl0w
Copy link
Member Author

CI is all green on my branch 😠 https://github.com/darkgl0w/fastify/actions/runs/1704685094

I will open a new PR when I figure out how to fix the flacky test in the custom-parser.test.js file.

@mcollina
Copy link
Member

CI fails on what appears to be an unrelated flacky test :d

Hmmm, ok at least 2 flacky tests spotted:

  • the 1st one is in the 404s.test.js file at line 1224. IMO it can be fixed by passing the tap timeout option :
- t.test('preHandler hook in setNotFoundHandler should be called when callNotFound', t => {

+ t.test('preHandler hook in setNotFoundHandler should be called when callNotFound', { timeout: 40000 }, t => {


  • the 2nd one is in the custom-parser.test.js file at line 1381.

It returns a 503 HTTP status code (Service Unavailable) instead of the 200 expected one. I will try to see if it can be fixed :d

Could you please open a couple of issues?

@darkgl0w
Copy link
Member Author

@mcollina > sure, the fix for the 404 test randomly timing out is included in this PR (I tried to reproduce it with the fix and it seems to be gone now)

And for the last one randomly returning a 503 HTTP Status I think I've got a fix. Currently running the test suite on a little Banana Pi to see if it works as it should ^^

Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

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

LGTM.

@Eomm Eomm added bugfix Issue or PR that should land as semver patch typescript TypeScript related labels Jan 17, 2022
@Eomm Eomm merged commit b0b0a7a into fastify:main Jan 17, 2022
@darkgl0w darkgl0w deleted the fix-this-typescript-definitions branch January 18, 2022 10:36
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bugfix Issue or PR that should land as semver patch typescript TypeScript related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A change in Fastify hook types broke our hooks
6 participants