-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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: Improve error experience #2954
Conversation
@@ -502,6 +502,9 @@ function fastify (options) { | |||
} else if (name === 'onReady') { | |||
this[kHooks].validate(name, fn) | |||
this[kHooks].add(name, fn) | |||
} else if (name === 'onRoute') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to move the setup for the onRoute
hooks due to the fact that was made for being async, in order to achieve register routes sync, the hooks should also be set in the same way. Please let me know your thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please target next
instead?
The description as I write this is "WIP". Therefore, I am marking it as a draft. Please mark it ready for review when you feel it is ready. |
3ae7553
to
c26bd0e
Compare
f9c50f2
to
4b313e1
Compare
lib/route.js
Outdated
// There's a bug when ignoreTrailingSlash !== true | ||
// See | ||
/* | ||
└── /prefix | ||
└── / (GET) | ||
/ (HEAD) | ||
└── / (GET) | ||
**/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty interesting error popping up only when ignoreTrailingSlash === true
, I didn't dig deep too much but was quite hard to detect. The third route comes out nowhere and the call stack is not really descriptive. I can take a look into it but not sure if it belongs directly to this PR as it happens before targeting to next
branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the error you are describing. As it happens on master
as well, open a fresh issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it happens on main
but caused me problems when pointing to next
.
What it does is create a third route out of nowhere, as described in the comment. A third route under /prefix//
is made, only if ignoreTrailingSlash = true
. I'll try to see if I can reproduce it on main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens in next
without this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incredibly I was not able to reproduce it on next
after a rebase. I'll try to figure out if stills reproducible or maybe was part of my changes before getting it stable
test/route.test.js
Outdated
@@ -173,7 +173,7 @@ test('route', t => { | |||
test('invalid schema - route', t => { | |||
t.plan(3) | |||
|
|||
const fastify = Fastify() | |||
const fastify = Fastify({ exposeHeadRoutes: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I set exposeHeadRoutes
to false in order to let the test pass with the correct route, as now if there's a schema validation error, the HEAD
route validation is being thrown first, just before the origin GET
route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you rebased this PR on top of next
? I think I fixed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, actually rebasing to next
popped up the issue. The routes are registered in order, first GET
and HEAD
as second, but as the schema build happens async, the first that comes up is always for the HEAD
one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that we should fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the issue, basically in order to receive first the error thrown by the schema validation of the first GET
route, is a need to pass the error to the done
function when receiving the error from the schema validation. Solved on 1ac09cc
test/route.test.js
Outdated
@@ -173,7 +173,7 @@ test('route', t => { | |||
test('invalid schema - route', t => { | |||
t.plan(3) | |||
|
|||
const fastify = Fastify() | |||
const fastify = Fastify({ exposeHeadRoutes: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you rebased this PR on top of next
? I think I fixed this.
test/schema-feature.test.js
Outdated
@@ -170,7 +170,7 @@ test('Cannot add schema for query and querystring', t => { | |||
|
|||
test('Should throw of the schema does not exists in input', t => { | |||
t.plan(2) | |||
const fastify = Fastify() | |||
const fastify = Fastify({ exposeHeadRoutes: false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if this is set to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error validates a throw on an unexisting schema reference, but the test is waiting for the error to be thrown on the original GET /:id
endpoint. But because of the order of the routes being loaded (first GET
, second HEAD
), the validation always fails on the HEAD
route throwing exactly the same error but just for the sibling route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test with exposeHeadRoutes: true
?
lib/route.js
Outdated
// There's a bug when ignoreTrailingSlash !== true | ||
// See | ||
/* | ||
└── /prefix | ||
└── / (GET) | ||
/ (HEAD) | ||
└── / (GET) | ||
**/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the error you are describing. As it happens on master
as well, open a fresh issue.
Basically, the same as the previous bugs. The sibling The tests is receiving a new error:
|
const { exposeHeadRoute } = opts | ||
const hasRouteExposeHeadRouteFlag = exposeHeadRoute != null | ||
const shouldExposeHead = hasRouteExposeHeadRouteFlag ? exposeHeadRoute : globalExposeHeadRoutes | ||
|
||
if (shouldExposeHead && options.method === 'GET' && !headRouteExists) { | ||
const onSendHandlers = parseHeadOnSendHandlers(opts.onSend) | ||
prepareRoute.call(this, 'HEAD', path, { ...opts, onSend: onSendHandlers }) | ||
} else if (headRouteExists && exposeHeadRoute) { | ||
warning.emit('FSTDEP007') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error stated on this comment was caused by the order of how the sibling HEAD
route was being registered for each GET
one.
As each GET
route was creating the sibling HEAD
one before registering a handler for the after
hook, the HEAD
route was registering first the hook handler, changing the order on how the validations should be done.
I moved the register of the sibling route on the after
route hook. Not sure if is the best or should do it after registering the hook handler and not within. Looking forward to having feedback 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me
test/404s.test.js
Outdated
fastify.listen(0, err => { | ||
t.ok(err) | ||
}) | ||
t.fail('setNotFoundHandler should not interfer duplicated route error') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"interfer" is not a word I recognize.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one and here solved in 5bfeeb 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still seeing this on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made another typo, sorry 😓
Done in 0659eb5
test/schema-feature.test.js
Outdated
@@ -170,7 +170,7 @@ test('Cannot add schema for query and querystring', t => { | |||
|
|||
test('Should throw of the schema does not exists in input', t => { | |||
t.plan(2) | |||
const fastify = Fastify() | |||
const fastify = Fastify({ exposeHeadRoutes: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is exposeHeadRoutes
is true
by default.
const fastify = Fastify({ exposeHeadRoutes: true }) | |
const fastify = Fastify() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0659eb5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a new test to not call onRoute
twice when duplicate routes?
lib/route.js
Outdated
// There's a bug when ignoreTrailingSlash !== true | ||
// See | ||
/* | ||
└── /prefix | ||
└── / (GET) | ||
/ (HEAD) | ||
└── / (GET) | ||
**/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It happens in next
without this PR?
a58880e
to
0d51d43
Compare
Sure, sounds good to me, has more sense as is indeed triggered before registering the route rather than after it. Let me update the documentation about it 👍
|
I added the changes for the documentation and remove the comments on the code, feel free to take another look and share feedback 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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. |
Fixes #2842
This PR tries to improve the error experience by changing to sync registering of routes along with a new error object for Fastify errors.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct