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

fix: handle middleware loading error #9458

Merged
merged 4 commits into from
Dec 19, 2023

Conversation

ematipico
Copy link
Member

Changes

Closes #9444

Testing

I locally tested the bug using the reproduction of the issue:

Screenshot 2023-12-18 at 14 05 11

Docs

N/A

Copy link

changeset-bot bot commented Dec 18, 2023

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 18, 2023
Copy link
Member

@MoustaphaDev MoustaphaDev left a 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(
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

@ematipico
Copy link
Member Author

Updated, now we get the overlay:

Screenshot 2023-12-18 at 17 20 29

Copy link
Member

@Princesseuh Princesseuh left a 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!

Copy link
Contributor

@lilnasy lilnasy left a 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!

@lilnasy
Copy link
Contributor

lilnasy commented Dec 18, 2023

Using cause may require bit of a touch up because it is ignored in the terminal.

@Princesseuh
Copy link
Member

Using cause may require bit of a touch up because it is ignored in the terminal.

Hmm, it shouldn't be:

if (err.cause) {

@lilnasy
Copy link
Contributor

lilnasy commented Dec 18, 2023

You're right! I'm thinking about prod errors, dev and build show causes.

Co-authored-by: Arsh <69170106+lilnasy@users.noreply.github.com>
@ematipico
Copy link
Member Author

@Princesseuh would it be possible to document #9458 (comment) somehow? Or even making it type aware, it was a guess game 😅

@ematipico ematipico merged commit fa3078c into main Dec 19, 2023
4 checks passed
@ematipico ematipico deleted the fix/handle-middleware-handling-error branch December 19, 2023 08:28
@astrobot-houston astrobot-houston mentioned this pull request Dec 19, 2023
export const MiddlewareCantBeLoaded = {
name: 'MiddlewareCantBeLoaded',
title: "Can't load the middleware.",
message: 'The middleware thrown an error while Astro was trying to loading it.',
Copy link
Member

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.',

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Middleware fails to work without showing an error
5 participants