Skip to content

fix(dev): apply headers from route rules for static assets - #2814

Closed
peterthenelson wants to merge 7 commits into
nitrojs:v2from
peterthenelson:v2
Closed

peterthenelson wants to merge 7 commits into
nitrojs:v2from
peterthenelson:v2

Conversation

@peterthenelson

Copy link
Copy Markdown

🔗 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
peterthenelson requested a review from pi0 as a code owner October 25, 2024 22:14
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
Copy Markdown
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
Comment thread src/core/dev-server/server.ts Outdated
@peterthenelson

Copy link
Copy Markdown
Author

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

@peterthenelson
peterthenelson requested a review from pi0 November 7, 2024 16:37
@pi0

pi0 commented Nov 7, 2024 •

Copy link
Copy Markdown
Member

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.

@prettypet2137

This comment was marked as off-topic.

@pi0

This comment was marked as off-topic.

@dataexcess

This comment was marked as off-topic.

@nitrojs nitrojs locked and limited conversation to collaborators May 12, 2025
@pi0
pi0 marked this pull request as draft May 12, 2025 22:02
@pi0 pi0 added the v2 label May 12, 2025
@pi0

pi0 commented Jan 7, 2026

Copy link
Copy Markdown
Member

Thanks for PR dear @peterthenelson and sorry left unattended. Moving to #3926

@pi0 pi0 closed this Jan 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

routeRules does not work in DEV mode for static files

4 participants