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 nestjs fastify platform warn #231

Merged
merged 3 commits into from
Sep 17, 2023

Conversation

KrainovSD
Copy link
Contributor

fixed warn:
FastifyDeprecation: Request#context property access is deprecated. Please use "Request#routeConfig" or "Request#routeSchema" instead for accessing Route settings. The "Request#context" will be removed in fastify@5.

dep:
"@nestjs/platform-fastify": "10.0.3",

Checklist

index.js Outdated Show resolved Hide resolved
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.

We used optional chaining in fastify/fastify-static#410, I'd like to use it here as well, just to be safe

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@0-don
Copy link

0-don commented Sep 17, 2023

having the same problem currently

Co-authored-by: Gürgün Dayıoğlu <[email protected]>
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.

I wanted to fast-track this PR but it is breaking older fastify versions now, after I applied the suggestions of @gurgunday

@gurgunday
Copy link
Member

Which version is it breaking?

The CI Looks Green To Me :)

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 17, 2023

Checkout this repo and checkout this PR. install fastify 4.20.0. Ensure that you have in node modules fastify version 4.20.0. run npm test.

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

Let's bump a major, they are cheap.

@gurgunday
Copy link
Member

Checkout this repo and checkout this PR. install fastify 4.20.0. Ensure that yo

Let me confirm you that master currently crashes as well at Fastify 4.20, I'll see what the issue is

@gurgunday
Copy link
Member

It's because of the test file that was changed in #230

global.test.js line 688 should be:
const crash = request.routeOptions?.config?.fail || request.routeConfig.fail

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.

Test file needs to be fixed as well

@gurgunday
Copy link
Member

gurgunday commented Sep 17, 2023

Let's bump a major, they are cheap.

@mcollina I wish we'd just done that from the beginning, but so far we've fixed breakages instead

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 17, 2023

@gurgunday

Can you please patch this PR accordingly?

Fix the tests locally and then in this page press the "." key on your keyboard. This opens the github editor for this Pr. You can then apply the changes and can commit as usual.

I am not at my pc, so can not do it myself.

@gurgunday
Copy link
Member

Fix the tests locally and then in this page press the "." key on your keyboard.

Wow, I didn't know this existed, so cool haha

Tests now pass on [email protected]

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 merged commit 52a7246 into fastify:master Sep 17, 2023
15 checks passed
@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 17, 2023

Now somebody has to release

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.

7 participants