Skip to content

feat: Handle new avvio error codes#3106

Merged
mcollina merged 7 commits intofastify:nextfrom
metcoder95:feat/handle-new-avvio-error
Jun 5, 2021
Merged

feat: Handle new avvio error codes#3106
mcollina merged 7 commits intofastify:nextfrom
metcoder95:feat/handle-new-avvio-error

Conversation

@metcoder95
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

} 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
Copy Markdown
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
Copy Markdown
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


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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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