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.

Sign up for GitHub

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

Jump to bottom

remove app.use and fix middleware tests #3506

Merged
merged 23 commits into from Dec 4, 2021
Merged

remove app.use and fix middleware tests #3506

merged 23 commits into from Dec 4, 2021

Conversation

genzyy
Copy link
Contributor

@genzyy genzyy commented Nov 30, 2021 •
edited

fix #3502

Deprecation of app.use() in next release.

  • removed app.use().
  • removed and modified the unit tests accordingly.

Checklist

mcollina
mcollina approved these changes Nov 30, 2021
View reviewed changes
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

climba03003
climba03003 requested changes Nov 30, 2021
View reviewed changes
Copy link
Member

@climba03003 climba03003 left a comment •
edited

Choose a reason for hiding this comment

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

Can you remove this part of code

fastify/lib/errors.js

Lines 108 to 115 in a174829

/**
* Middlewares
*/
FST_ERR_MISSING_MIDDLEWARE: createError(
'FST_ERR_MISSING_MIDDLEWARE',
'You must register a plugin for handling middlewares, visit fastify.io/docs/latest/Middleware/ for more info.',
500
),

For the TypeScript error, it must be fixed in pino side.

@genzyy
Copy link
Contributor Author

genzyy commented Nov 30, 2021

Can you remove this part of code
Removed it!

For the TypeScript error, it must be fixed in pino side.

Yeah that's why npm run test fails for now.

climba03003
climba03003 approved these changes Nov 30, 2021
View reviewed changes
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.

@climba03003 climba03003 mentioned this pull request Nov 30, 2021
Merged
RafaelGSS
RafaelGSS approved these changes Nov 30, 2021
View reviewed changes
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.

@genzyy
Copy link
Contributor Author

genzyy commented Dec 2, 2021

I forgot to change Middleware.md which is the documentation for app.use(). What should I put here?

@mcollina
Copy link
Member

mcollina commented Dec 2, 2021

Just remove it. Make sure of

  1. .use is not documented elsewhere
  2. middie and fastify-express as listed inside the core plugins.

genzyy
genzyy commented Dec 2, 2021
View reviewed changes
docs/Plugins.md Outdated Show resolved Hide resolved
mcollina
mcollina reviewed Dec 2, 2021
View reviewed changes
docs/Migration-Guide-V3.md Outdated Show resolved Hide resolved
docs/Middleware.md Show resolved Hide resolved
Eomm
Eomm reviewed Dec 2, 2021
View reviewed changes
docs/Migration-Guide-V4.md Outdated Show resolved Hide resolved
@genzyy @Eomm
WIP: Update docs/Migration-Guide-V4.md
a4cfc82 
Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>
@genzyy
Copy link
Contributor Author

genzyy commented Dec 3, 2021

Is there anything more I can add/change here?

climba03003
climba03003 requested changes Dec 3, 2021
View reviewed changes
docs/Plugins.md Outdated Show resolved Hide resolved
docs/Migration-Guide-V4.md Outdated Show resolved Hide resolved
genzyy and others added 2 commits December 3, 2021 16:18
@genzyy @climba03003
Update docs/Migration-Guide-V4.md
c83a4ad 
- add links to `middie` and `fastify-express`.
- add documentation link to fastify hooks.

Co-authored-by: KaKa <climba03003@gmail.com>
@genzyy
remove unnecessary links
82b1a80
climba03003
climba03003 approved these changes Dec 3, 2021
View reviewed changes
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

@mcollina
Copy link
Member

mcollina commented Dec 4, 2021 •
edited

Can you rebase on top of next? CI is failing in this PR for unrelated things

@genzyy
Copy link
Contributor Author

genzyy commented Dec 4, 2021

Pulled next branch, rebased fix-3502 on top of next.

@mcollina mcollina merged commit e488a09 into fastify:next Dec 4, 2021
@Eomm Eomm mentioned this pull request Dec 14, 2021
Closed
2 tasks
@github-actions
Copy link

github-actions bot commented Dec 5, 2022

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 Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mcollina mcollina mcollina approved these changes

@delvedor delvedor delvedor left review comments

@Eomm Eomm Eomm left review comments

@climba03003 climba03003 climba03003 approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@genzyy @mcollina @delvedor @Eomm @climba03003 @RafaelGSS