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

fix(plugins): mixing async and callback style now returns an error #5141

Merged
merged 3 commits into from
Nov 9, 2023
Merged

fix(plugins): mixing async and callback style now returns an error #5141

merged 3 commits into from
Nov 9, 2023

Conversation

giuliowaitforitdavide
Copy link
Contributor

Closes #5112, opened as discussed in #5139

Checklist

@giuliowaitforitdavide giuliowaitforitdavide mentioned this pull request Nov 5, 2023
Merged
4 tasks
metcoder95
metcoder95 reviewed Nov 6, 2023
View reviewed changes
lib/pluginUtils.js Outdated
@@ -134,7 +135,14 @@ function registerPluginName (fn) {
this[kRegisteredPlugins].push(name)
}

function checkPluginHealthiness (fn) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's just check this gets in sync after #5139 gets merged into main 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'm marking the PR as draft

lib/errors.js Outdated
@@ -434,6 +434,12 @@ const codes = {
'FST_ERR_PLUGIN_NOT_PRESENT_IN_INSTANCE',
"The decorator '%s'%s is not present in %s"
),
FST_ERR_PLUGIN_INVALID_ASYNC_HANDLER: createError(
'FST_ERR_PLUGIN_INVALID_ASYNC_HANDLER',
'Async function has too many arguments. Async plugin should not use the \'done\' argument.',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Async function has too many arguments. Async plugin should not use the \'done\' argument.',
'The %s plugin being registered mixes async and callback styles. Async plugin should not mix async and callback style.',

@giuliowaitforitdavide giuliowaitforitdavide marked this pull request as draft November 6, 2023 09:27
@giuliowaitforitdavide giuliowaitforitdavide marked this pull request as ready for review November 6, 2023 10:13
metcoder95
metcoder95 approved these changes Nov 6, 2023
View reviewed changes
Copy link
Member

@metcoder95 metcoder95 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 7, 2023
View reviewed changes
Copy link
Member

@climba03003 climba03003 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you rebase your changes based on the latest next branch?

@giuliowaitforitdavide
Copy link
Contributor Author

Could you rebase your changes based on the latest next branch?

Done, I also fixed failing tests on the next branch but I'm not sure about the coverage

@climba03003
Copy link
Member

Done, I also fixed failing tests on the next branch but I'm not sure about the coverage

Sorry to bother you, I have cleanup the unclean merge from the next branch. Could you try again to rebase on top of it?

climba03003
climba03003 approved these changes Nov 8, 2023
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, I believe the coverage is not related to this PR.
It is related to the Symbol.asyncDispose

@giuliowaitforitdavide
Copy link
Contributor Author

LGTM, I believe the coverage is not related to this PR.
It is related to the Symbol.asyncDispose

Ok, so I won't fix it in this PR. I thought it was my mistake

@Eomm Eomm added v5.x Issue or pr related to Fastify v5 notable-change A change that should be explicitly outlined in release notes and migration guides labels Nov 9, 2023
@Eomm
Copy link
Member

Eomm commented Nov 9, 2023

Merging - coverage is a next issue

@Eomm Eomm merged commit 52bbc37 into fastify:next Nov 9, 2023
29 of 31 checks passed
@giuliowaitforitdavide giuliowaitforitdavide deleted the fix/#5112-next branch November 17, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metcoder95 metcoder95 metcoder95 approved these changes

@Eomm Eomm Eomm approved these changes

@climba03003 climba03003 climba03003 approved these changes

Assignees
No one assigned
Labels
notable-change A change that should be explicitly outlined in release notes and migration guides v5.x Issue or pr related to Fastify v5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@giuliowaitforitdavide @climba03003 @Eomm @metcoder95