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(dev): apply headers from route rules for static assets #2814

Open
wants to merge 7 commits into
base: v2
Choose a base branch
from

Conversation

peterthenelson
Copy link

πŸ”— Linked issue

#2749

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • [x ] 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

As detailed in the linked bug, routeRules fail to apply to public assets when using the dev server. The reason this happens is, IIUC, that the handler for routeRules runs in the reloadable worker (like the majority of the server logic), while the public asset serving happens locally in the app created in dev-server/server.ts. Requests for public assets get served without ever proxying anything to the worker.

I solve this by adding a handler to the dev server itself that partially duplicates some of the logic in route-rules.ts. Some notes:

  • This only applies the headers from routeRules. The actual handler in the worker also allows for redirects and proxying. It's not clear to me whether it makes any sense to apply those to public assets, and in the case of proxying, it requires the "localFetch" object.
  • This redundantly sets the headers in cases where the dev server was already setting them (i.e., the non public assets). I considered a couple alternatives, but they would either require duplicating or inverting the URL-path-to-file-path logic from the "send" package (which is what "serve-static" uses).

Anyway, it's totally possible I'm going about this the wrong way, but that's my reasoning for why I did it this way.

Resolves #2749

πŸ“ Checklist

  • [x ] I have linked an issue or discussion.
  • I have updated the documentation accordingly.

- These otherwise fail to be applied to public
  assets (see nitrojs#2749).
- This change replies them redundantly in the
  other cases (where they were already being
  applied), but the alternative would require
  duplicating or reversing the URL / file path
  logic from the "send" package.

fixes nitrojs#2749
peterthenelson added a commit to peterthenelson/fiolin that referenced this pull request Oct 26, 2024
- Using nitro so that I can use the dev servers for local development
  and testing of the CSPs, and then it should make Cloudflare Pages
  deployments pretty easy.
- Unfortunately I had to fix a bug in the nitro dev server; hopefully I
  can get nitrojs/nitro#2814 merged (or another approach to fixing it if
  the maintainers don't like that one).
- Added functionality to run 3p scripts (you can test that this works
  by using the fake3p server--see README.md).
- I think current version of CSPs has the properties I want, but I might
  need to change them when adding WASM.
@peterthenelson
Copy link
Author

@pi0 let me know if there's anything else I should do before you review this. Also, thanks for all your work on the many unjs projects; I've been finding them really useful :)

@pi0 pi0 changed the title fix: apply headers from routeRules in dev server fix(dev): apply headers from routeRules in dev server Oct 31, 2024
@pi0 pi0 changed the title fix(dev): apply headers from routeRules in dev server fix(dev): apply headers from route rules for static assets Oct 31, 2024
src/core/dev-server/server.ts Outdated Show resolved Hide resolved
@peterthenelson
Copy link
Author

I had a followup question (see thread) before I make the requested changes.

@pi0
Copy link
Member

pi0 commented Nov 7, 2024

Added tests, two issues:

  • path passed inside middleware omits base /build/test.txt gets /test.txt therefore rules isn't matching. (we need to find solution)
  • Currently, nitro has an implicit behavior that ignores maxAge of public assets in development mode which means this patch can cause behavior change (long term asset caches in dev). I am thinking for Nitro v2, we should avoid setting cache-control from route rules to avoid possible regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

routeRules does not work in DEV mode for static files
2 participants