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

[frameworks] Fix astro default routes since it is not SPA #8200

Merged
merged 7 commits into from
Nov 29, 2022
Merged

Conversation

lucleray
Copy link
Member

The default routes for Astro are redirecting all non-existing files to index.html, which means that the 404.html page is not used, and instead index.html is shown for all not found pages.

Astro outputs files for each page (ie. about.html, blog.html, ...) so the {handle: 'filesystem'} route should be enough to route all existing pages correctly.

The missing part to ship this fix is to answer the following question: can we safely assume that Astro will always output a 404.html file?

Internal ref: https://github.com/vercel/customer-issues/issues/638

@lucleray lucleray requested a review from okikio July 22, 2022 08:13
okikio
okikio previously requested changes Jul 22, 2022
Copy link
Contributor

@okikio okikio left a comment

Choose a reason for hiding this comment

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

We need a way to have the redirects work but still have the 404 pages work, ideally leaving it to Astro to decide. Yesterday I did some research into it, and the problem is much larger in scope on the Astro side, I'll see if I can get a possible fix out, but if not this might be the only solution we have for now.

packages/frameworks/src/frameworks.ts Show resolved Hide resolved
@EndangeredMassa
Copy link
Contributor

@okikio @lucleray what's the path forward on this work?

@lucleray
Copy link
Member Author

I'm going to close it for now. If it still makes sense to ship it, we can reopen.

@lucleray lucleray closed this Nov 15, 2022
@lucleray lucleray deleted the astro-404-fix branch November 15, 2022 09:19
@okikio okikio restored the astro-404-fix branch November 24, 2022 22:10
@delucis
Copy link
Contributor

delucis commented Nov 24, 2022

Hi everyone, Astro core maintainer here 👋

I was about to make a PR more or less identical to this and thought it might be best to discuss here first — maybe we can re-open.

We currently document a weird hack in the Astro Vercel deploy guide, where setting "cleanUrls": true in vercel.json makes custom 404 pages display for static builds, but I’ve never understood why that would work. Would be cool if that wasn’t needed.

There seems to be a bit of confusion between “SPA” mode and “SSR” in the discussion above, Astro doesn’t have an SPA-mode where you would route everything to /index.html and let client-side routing take over, but does support SSR via its Vercel adapter.

In SSR mode, Astro’s adapter outputs routing configuration so I believe this defaultRoutes is ignored. In static mode, Astro builds an HTML file per page, so the filesystem routing should work, we just need to handle the special 404 case.

The missing part to ship this fix is to answer the following question: can we safely assume that Astro will always output a 404.html file?

The answer here is no: if a user does not create a src/pages/404.astro file, there won’t be a dist/404.html to route to. I assume in this case Vercel displays a generic 404 page?

@okikio okikio reopened this Nov 24, 2022
@styfle styfle changed the title [frameworks] Fallback to 404.html in astro default routes [frameworks] Fix astro default routes since it is not SPA Nov 29, 2022
@TooTallNate TooTallNate marked this pull request as ready for review November 29, 2022 19:42
@TooTallNate TooTallNate added the semver: patch PR contains bug fixes label Nov 29, 2022
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

I tried the example from the original PR #7747 as well as the latest example and both are SSG style, so I don't know why routes were added to begin with.

I discussed with @TooTallNate and we found that removing these routes (and fixing the asset header) would make astro work perfectly, thanks!

@styfle styfle requested a review from okikio November 29, 2022 19:43
@styfle styfle added the pr: automerge Automatically merge the PR when checks pass label Nov 29, 2022
@styfle styfle merged commit 2a4e066 into main Nov 29, 2022
@styfle styfle deleted the astro-404-fix branch November 29, 2022 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: frameworks pr: automerge Automatically merge the PR when checks pass semver: patch PR contains bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants