Repository navigation
[FE-1026] Cache immutable self-hosted UI assets - #4011
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
5b2e646 to
3d015b9
Compare
| handler := http.FileServer(http.FS(assets)) | ||
| return echo.WrapHandler(handler) | ||
| return echo.WrapHandler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if strings.HasPrefix(r.URL.Path, "/_app/immutable/") && !strings.HasSuffix(r.URL.Path, "/") { |
There was a problem hiding this comment.
Does r.URL.Path take into account publicPath if they set that?
| func (w *immutableAssetResponseWriter) WriteHeader(status int) { | ||
| if !w.wroteHeader { | ||
| if status >= http.StatusOK && status < http.StatusMultipleChoices { | ||
| w.Header().Del("Set-Cookie") |
There was a problem hiding this comment.
This removes every Set-Cookie header on the response. It looks like deleting "all cookies" and deleting "_csrf" do the same thing today, but might be worth narrowing the delete to future proof this.
There was a problem hiding this comment.
Actually, do we need to mint the cookie on these routes at all 🤔
There was a problem hiding this comment.
CSRF middleware runs globally, so yeah these were getting attached to immutable assets as well in my testing.
If we narrow it via this deletion line, I think we need to do more than just restrict the deletion to the _csrf cookie. The first thing that comes to mind in the future would be session refresh or auth cookie. If we keep these cookies and make the asset immutable, then that's the same problem, arguably worse.
I think the proper future narrowing solution may be to prevent auth/session middleware from running on immutable static routes at all (if possible).
There was a problem hiding this comment.
Sorry, just re-read your second comment: no, we don't need mint the cookie on these routes at all.
|
|
||
| func (w *immutableAssetResponseWriter) WriteHeader(status int) { | ||
| if !w.wroteHeader { | ||
| if status >= http.StatusOK && status < http.StatusMultipleChoices { |
There was a problem hiding this comment.
Should we account for a 304 here?
There was a problem hiding this comment.
I was trying to track down whether or not a 304 is actually even reachable in this configuration.
My understanding is that go strips metadata including timestamps for embedded assets.
Either way, this is a safe follow up I think. Will make another PR to address this.
There was a problem hiding this comment.
@laurakwhit Great catch! there actually was an edge case for this: when uiAssetsPath is set, we're no longer using Go's embedded assets, and filesystem assets do respond with timestamps.
Auto-generated version bump from 2.56.0 to 2.57.0 Bump type: minor go.temporal.io/api: upgraded since last version (minimum minor) Changes included: - [`e15c805a`](e15c805) fix(nexus): show error alert when starting a standalone Nexus operation fails (FE-1083) (#4026) - [`dad76961`](dad7696) feat(io): add button and button-link components (FE-1090) (#4028) - [`8ecbc7e5`](8ecbc7e) Add tooltip to truncated filter chips (#4034) - [`2a687175`](2a68717) Allow text selection in workflow descriptions in relationships (#4035) - [`4c440b26`](4c440b2) refactor(io): centralize chip and badge styling (FE-1095) (#4029) - [`beccfa52`](beccfa5) Reuse badge styling and grouped corners for tags (FE-1093) (#4030) - [`e6cece8e`](e6cece8) Add reusable native select with custom arrow styling (#4036) - [`40108cfc`](40108cf) Dependency Inject Authz decisions through the provider (#4031) - [`ea31abca`](ea31abc) Add persistent environment banner with preset colors (#4039) - [`bd5c6600`](bd5c660) Fix timeline summary typography (#4043) - [`0ad513c2`](0ad513c) fix(schedules): preserve existing headers when editing (#4051) - [`7eb3a834`](7eb3a83) Hide focus outline on main content landmark (#4052) - [`0ee98978`](0ee9897) Add client-side sorting to API-paginated tables (#4037) - [`c80a75e8`](c80a75e) Wrap long link values in expanded event details (#4053) - [`51606c50`](51606c5) Remove trailing space inside Link (#4054) - [`23e6c4d1`](23e6c4d) Improve sorting performance for large workflow tables (#4055) - [`45015193`](4501519) [FE-1026] Cache immutable self-hosted UI assets (#4011) - [`25a98476`](25a9847) Upgrade build tooling to Vite 8 (#4038) - [`b911d0a6`](b911d0a) [FE-1026] Cache immutable 304 assets (#4056) - [`fa6e6ccc`](fa6e6cc) Bump go.temporal.io/api to v1.64.0 in the server (#4059) - [`a654962e`](a654962) Bump Node to 22.23.4 and Go to 1.26.10 for security fixes (#4058) Co-authored-by: rossnelson <146089+rossnelson@users.noreply.github.com>

https://linear.app/temporal/issue/FE-1026/investigate-multiple-js-files-downloading
Summary
Self-hosted Temporal UI was serving SvelteKit's content-addressed
/_app/immutable/*assets from the Go ui-server without cache headers, causing browsers to transfer the same JavaScript again on reload. The Go static handler now sendsCache-Control: public, max-age=31536000, immutablefor successful immutable file responses.This PR supersedes the delivery mechanism introduced by #3128. That PR established the correct cache policy in
hooks.server.ts, but this repository builds with@sveltejs/adapter-static: exported files do not execute SvelteKit server hooks when the Go ui-server serves them. This PR removes that unreachable hook mutation and its dedicated tests while preserving the hook's resolve pass-through and UUID polyfill, then applies the same policy at the serving layer that owns the response.Delivery-path findings
@sveltejs/adapter-vercel6.3.4 generates its own/_app/immutable/one-year cache rule and ano-storerule for missing immutable assets. Cloud UI therefore does not depend on this hook.Safety boundaries
/_app/immutable/file responses receive the public immutable directive.200and206immutable file responses are cacheable;301,404, and416responses are not._csrfcookie andVary: Cookiebefore the static handler writes its response. Cacheable immutable responses therefore removeSet-Cookieand only theCookietoken fromVary, while preserving unrelated tokens such as Gzip'sAccept-Encoding.Non-goals
SvelteKit adapter-node also supports
HEAD, content-hash ETags, conditional304responses, and precompressed asset variants. Those are independent serving optimizations and are not required to correct the missing cache policy, so this PR does not add them to ui-server.Verification
Vary: Accept-Encoding, Cookiebefore the fix.Set-Cookie, noVary: Cookie, and still hasVary: Accept-Encoding.206ranges, immutable directories, redirects, missing files,416ranges, and non-immutable assets.go vetandgo test ./server/...passed.pnpm lintandpnpm checkpassed with existing warnings only.pnpm test -- --runpassed: 284 files, 3,711 tests, 2 skipped.pnpm build:serverpassed.