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: better dev routing with base using middleware #3942

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

AllanChain
Copy link
Contributor

Changes

Use a base middle for better base handling in dev.

The previous dev routing design is not very friendly with a base path: it has to prioritize the custom 404 page over redirecting to public files, causing some problems:

Here I'm using the middleware approach inspired by Vite: return 404, or strip the base path and continue. In other words, the logic for returning 404 based on the base path is moved into the middleware. Thus the public files are correctly handled by the static serving middleware without the need to redirect.

However, just like Vite and the previous approach, requests for public files not starting with the base path are not responded to with 404.

Testing

Added some tests.

Tested with my real project with base path and custom 404 enabled.

Docs

No need.

@changeset-bot
Copy link

changeset-bot bot commented Jul 16, 2022

🦋 Changeset detected

Latest commit: c2bdcd2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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 Jul 16, 2022
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.

This makes a lot of sense to me, thanks for the PR!

Hoping somebody more familiar with this original logic can review as well.

@matthewp
Copy link
Contributor

Can you provide some examples of some URLs and how they would previous resolve and how they will resolve now? That would help understand what you have changed here.

http://localhost:3000/blog/blog/blog/blog/ will redirect to http://localhost:3000/blog/

Why? It seems like http://localhost:3000/blog/blog/blog/blog/ should be a 404 if there isn't a blog/blog/blog/blog.astro file.

Copy link
Contributor

@tony-sull tony-sull left a comment

Choose a reason for hiding this comment

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

Very nice, glad to see that redirect hack go away! 🎉

Thanks for filing this PR and including clean test coverage for it!

@AllanChain
Copy link
Contributor Author

Why? It seems like http://localhost:3000/blog/blog/blog/blog/ should be a 404 if there isn't a blog/blog/blog/blog.astro file.

If you look at the code here:

// HACK: redirect without the base path for assets in publicDir
const redirectTo =
req.method === 'GET' &&
config.base !== '/' &&
pathname.startsWith(config.base) &&
pathname.replace(config.base, '/');
if (redirectTo && redirectTo !== '/') {
const response = new Response(null, {
status: 302,
headers: {
Location: redirectTo,
},
});
await writeWebResponse(res, response);
return;
}

/blog/blog/blog/blog/ starts with the base /blog/, so redirectTo has the value /blog/blog/blog/, so the browser will request /blog/blog/blog/. Then /blog/blog/blog/ becomes /blog/blog/, and so on.

Can you provide some examples of some URLs and how they would previous resolve and how they will resolve now? That would help understand what you have changed here.

Example 1: http://localhost:3000/blog/blog/blog/blog/

  • Previous: redirect to http://localhost:3000/blog/
  • Here: return 404

Example 2: http://localhost:3000/blog/image.png (image.png exists)

  • Previous:
    • without 404.astro: redirect to http://localhost:3000/image.png
    • with 404.astro: return custom 404 page
  • Here: returns the image

Example 3: http://localhost:3000/blog/image.png (image.png not exists)

  • Previous:
    • without 404.astro: redirect to http://localhost:3000/image.png and 404
    • with 404.astro: return custom 404 page
  • Here: custom for default 404 page

@natemoo-re
Copy link
Member

Thanks @AllanChain, those specific cases seem much better in this PR! Really appreciate the contribution.

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.

4 participants