-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix: handle middleware loading error #9458
Conversation
🦋 Changeset detectedLatest commit: 47fe703 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to 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.
LGTM!
try { | ||
const module = await moduleLoader.import(MIDDLEWARE_MODULE_ID); | ||
return module; | ||
} catch { | ||
} catch (e: any) { | ||
logger.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.
Is there a specific reason for this to be a log instead of an actual throw?
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.
No particular reason. Should we throw an astro error? What do you think?
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'd say so! Presumably if the middleware fails to load, we'd like the build to fail? I'd just wonder in dev if it would work correctly? Would it crash the dev server? If so, maybe it'd be possible to handle it correctly using the method Matthew added not too long ago.
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.
Optimally, we should catch the error, rethrow an error (Astro couldn't load the middleware...) and put the original error in the cause
. However, I think this is still a major improvement over silently ignoring the error, ha!
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.
Much needed fix, thanks!
Using cause may require bit of a touch up because it is ignored in the terminal. |
Hmm, it shouldn't be: astro/packages/astro/src/core/messages.ts Line 283 in 047d285
|
You're right! I'm thinking about prod errors, dev and build show causes. |
Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
@Princesseuh would it be possible to document #9458 (comment) somehow? Or even making it type aware, it was a guess game 😅 |
export const MiddlewareCantBeLoaded = { | ||
name: 'MiddlewareCantBeLoaded', | ||
title: "Can't load the middleware.", | ||
message: 'The middleware thrown an error while Astro was trying to loading it.', |
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.
Hi @ematipico ! There's a tiny language error here! Can you please update to whichever feels right to you?
message: 'The middleware threw an error while Astro was trying to loading it.',
Changes
Closes #9444
Testing
I locally tested the bug using the reproduction of the issue:
Docs
N/A