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

feat: Handle new avvio error codes #3106

Merged
merged 7 commits into from
Jun 5, 2021

Conversation

metcoder95
Copy link
Member

Closes #2842

Note:

  • This PR depends on the next major release of avvio until then is pointing out to fastify/avvio#next branch for dependency.

Checklist

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 mcollina added this to the v4.0.0 milestone May 28, 2021
@mcollina mcollina added the semver-major Issue or PR that should land as semver major label May 28, 2021
Copy link
Member

@jsumners jsumners left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just need to rebase a bit.

fastify.js Outdated Show resolved Hide resolved
@@ -277,7 +277,7 @@ t.test('onReady cannot add lifecycle hooks', t => {
fastify.addHook('onRequest', (request, reply, done) => {})
} catch (error) {
t.ok(error)
t.equal(error.message, 'root plugin has already booted')
t.equal(error.message, 'Root plugin has already booted')
Copy link
Member

Choose a reason for hiding this comment

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

add a check on the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! done in cbb0e73

@@ -845,8 +845,8 @@ test('plugin metadata - dependencies (nested)', t => {
}
})

test('pluginTimeout', t => {
t.plan(2)
test('pluginTimeout', { only: true }, t => {
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
test('pluginTimeout', { only: true }, t => {
test('pluginTimeout', t => {

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in cbb0e73

@@ -855,12 +855,14 @@ test('pluginTimeout', t => {
})
fastify.ready((err) => {
t.ok(err)
t.equal(err.code, 'ERR_AVVIO_PLUGIN_TIMEOUT')
t.equal(err.message,
"fastify-plugin: Plugin did not start in time: 'function (app, opts, done) { -- // to no call done on purpose'. You may have forgotten to call 'done' function or to resolve a Promise")
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a test with a named function too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, made a new one in cbb0e73

@metcoder95 metcoder95 force-pushed the feat/handle-new-avvio-error branch from cbb0e73 to b176784 Compare June 1, 2021 22:05
@metcoder95 metcoder95 requested a review from Eomm June 1, 2021 22:06
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.

good work, just a nit

@@ -277,7 +277,9 @@ t.test('onReady cannot add lifecycle hooks', t => {
fastify.addHook('onRequest', (request, reply, done) => {})
} catch (error) {
t.ok(error)
t.equal(error.message, 'root plugin has already booted')
t.equal(error.message, 'Root plugin has already booted')
// TODO: look where the error pops up
Copy link
Member

Choose a reason for hiding this comment

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

why is this TODO here? Can you resolve it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm taking a look right now. I just wanted to push the changes already made for keeping track of the progress 👍

Also, this is quite weird. The thing is that it seems it throws sync, and for instance, I'm not able to fully catch it, do you have any idea what I'm missing?

docs/Errors.md Outdated
<a name="FST_ERR_PLUGIN_TIMEOUT"></a>
#### FST_ERR_PLUGIN_TIMEOUT

Plugin did not start in time.
Copy link
Member

Choose a reason for hiding this comment

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

Could we share the timeout default here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in d315fed

@mcollina mcollina merged commit c631bb7 into fastify:next Jun 5, 2021
@github-actions
Copy link

github-actions bot commented Jun 6, 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 Jun 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-major Issue or PR that should land as semver major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants