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 missing CSS when 404 server Response redirects to a custom 404 page #7946

Merged

Conversation

andremralves
Copy link
Contributor

Changes

Closes #7910

The regression was introduced here #7703 because of a typo.

This PR changes the variable used in #createRenderContext back to the correct one: errorRouteData.

The change was simple (but really hard to find) and I also added some tests to prevent this error from happening again.

Testing

One test to check imports in 404 redirections to a custom 404 page using contentCollections.

Docs

n/a

@changeset-bot
Copy link

changeset-bot bot commented Aug 4, 2023

🦋 Changeset detected

Latest commit: 77aae41

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 the pkg: astro Related to the core `astro` package (scope) label Aug 4, 2023
@andremralves andremralves changed the title Fix/missing css import on 404 redirect Fix missing CSS when 404 server Response redirects to a custom 404 page Aug 4, 2023
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this. Besides the merge conflicts, this lgtm

@andremralves
Copy link
Contributor Author

I will fix the conflicts.

@natemoo-re
Copy link
Member

Good catch! I'm guessing the conflicts are coming from #7754 which was merged quite recently. Please check out that PR. It might have fixed this issue already?

@andremralves
Copy link
Contributor Author

andremralves commented Aug 4, 2023

Good catch! I'm guessing the conflicts are coming from #7754 which was merged quite recently. Please check out that PR. It might have fixed this issue already?

I also thought your PR might have fixed the issue, so I made some tests, but the problem was still showing up.

I believe that the line const finalRouteData = routeData ?? errorRouteData; can be the problem, shouldn't we always use errorRouteData to create the render context as const mod will aways use errorRouteData?

In my tests, the problem was being caused because a route other than /404 was being passed to #createRenderContext inside the finalRouteData constant.

I just could reproduce this error using ssr with contentCollections. An example can be found in the tests that I made.

@andremralves
Copy link
Contributor Author

andremralves commented Aug 4, 2023

I made a new commit merging the branches and changing a little bit the behavior of #renderError, I believe the issue is fixed now. But a careful review would be good because the logic was different.

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Thanks for the thorough update and adjusting to fix the updated logic! This change makes a lot of sense and looks good to me!

@natemoo-re natemoo-re merged commit 9d00700 into withastro:main Aug 7, 2023
@astrobot-houston astrobot-houston mentioned this pull request Aug 7, 2023
@paulrudy
Copy link
Contributor

paulrudy commented Aug 7, 2023

Thanks for finding and addressing this!

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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 page missing CSS when 404 server Response redirects to static custom 404 page
4 participants