feat(static): support after ordering and warn when fallthrough: false blocks REST#1574
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces 'before' and 'after' configuration options to the static file serving handler, allowing users to customize its position in the HTTP middleware chain. It also adds validation for these options, a warning when a non-fallthrough static handler might block REST API routes, and corresponding unit tests. The reviewer noted that because 'before' and 'after' are only evaluated at startup, live-reload changes to these options will be silently ignored, and suggested explicitly requesting a restart when they change.
|
Reviewed; no blockers found. |
…tically Matches HarperFast/harper#1574, which now requests a component restart when before/after change at runtime. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kriszyp
left a comment
There was a problem hiding this comment.
Approving — nice fix for the static-shadows-REST GET footgun; after ordering resolves once at registration (no per-request cost) and the fallthrough:false warning is accurate.
One question I'm curious about: what's the use case where you'd actually reach for after here — is it for a specific static-vs-REST layout, or a general escape hatch? And given this change, is the current default ordering still the right one for the common case, or does this suggest the default should change so people don't need after in the first place?
— 🤖 KrAIs (Kris's review assistant)
|
There are legitimate cases for both. For most Harper apps, static was for purely known, static asset serving. From a known map, those requests can be served the fastest, and they help speed up site metrics. It's a cheap thing to check, should be served before authentication, and then you check your meaty resources if they want to handle things. But with SPAs that want to use real looking URLs and query string parameters for users, a 404 fallback to a known index.html (and scripted client side route loading) is a very common approach to serving content. The static plugin couldn't handle this situation. We opted to push people to use hash based routing, which does have advantages. But with such a simple change, we can let people do what they want. They may not have a choice over the URLs they must handle. |
Is this really the ideal way to handle mapping a set (a glob?) of URLs ("real looking") to a static file (pass throug auth, REST, 404, fallback handler)? Should we add support for URL glob -> file path for explicit and fast mapping? |
…lse` blocks REST The static handler registers before authentication by default (and before the REST handler, which runs after authentication), so a `fallthrough: false` catch-all - the documented recipe for history-mode SPAs - answers every unmatched GET itself, including GETs for exported REST resources. The application's REST API becomes unreachable over GET with no signal to the user about why. - Support `after` in the static options (e.g. `after: 'rest'`) so the API is matched first and only unmatched URLs receive the `notFound` fallback. When `after` is given, the default `before: 'authentication'` hoist is suppressed - combining them would create an ordering cycle. - Formalize `before: false` to clear the default constraint without adding a new one (registration order applies). - Validate `before`/`after` and document them in the plugin JSDoc. - Warn at startup (and on live reload of `fallthrough`) when `fallthrough: false` is used in the default pre-REST position, with the `after: 'rest'` remedy in the message. - Describe the static component, including the new options, in config-app.schema.json so editors get completion and validation. Verified end-to-end against a Vite SPA app with an exported resource: with `after: 'rest'`, GET /Greeting/ returns the resource's JSON while unmatched deep links still receive the index.html fallback - history-mode routing and the REST API coexist. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Scope only auto-restarts on unhandled option changes when the plugin has no 'change' listener of its own - static has one, and before/after are consumed once at registration, so a live edit to them was silently ignored. Request a component restart so the middleware chain is rebuilt with the new ordering. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Treat a bare `before:`/`after:` key (YAML null) as unset, preserving the pre-validation behavior where null fell back to the default hoist. - Warn on unresolved `before`/`after` names: topoSort silently drops references that match no registered handler (e.g. a typo, or the legacy `REST` config key), so the middleware chain now reports them on first dispatch — build time would false-positive, since chains are rebuilt as each component registers. - `warnIfBlockingRest` reads live option values (a config save can change `fallthrough` and the ordering options together) and also fires for an explicit `before: 'authentication'`, which is the same blocking position. - Flatten the nested ternary in the registration options. - Schema: `additionalProperties: false` on the static block (matching the sibling component blocks) and `minLength: 1` on `before`/`after`. - Tests: fake scope's fireChange now matches the real OptionsWatcher signature, plus coverage for null/empty ordering values, before+after combinations, and the unresolved-reference diagnostic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
611c96b to
6865ecc
Compare
|
@kriszyp specifying your paths would be more performant, yes. But you're contending with a lot of people that want to do it with a 404, and have done it with 404 for a very long time. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
The static handler registers
before: 'authentication'by default (runFirst: trueprior to 5.1), and the REST handler runsafter: 'authentication'— so static always answers GETs first. That makes the documented history-mode SPA recipe a footgun:This catch-all answers every unmatched GET itself — including GETs for exported REST resources — so the application's REST API silently becomes unreachable over GET (
GET /MyResource/returns index.html with a 200). All four create-harper SPA templates shipped this config; create-harper#109 removes it there and steers users toward hash routing, but Harper itself should support the history-mode pattern correctly and warn about the trap.With this PR
One added line makes the recipe work as everyone assumed it did — the API is matched first, and the fallback only catches what's left:
History-mode routing and the REST API coexist. And anyone still running the old config now gets a startup warning in the log naming this exact fix.
Changes
afteroption on static (e.g.after: 'rest'): positions the handler after the named middleware so API routes match first and only unmatched URLs receive thenotFoundfallback. Whenafteris given, the defaultbefore: 'authentication'hoist is suppressed — combining them would create an ordering cycle (static < authentication < rest < static), which would fall back to registration order with a warning.before: falseis now a documented, validated way to clear the default constraint without adding a new one (registration order applies). This previously worked only by accident of the falsy check intopoSort.before/aftervalues, matching the style of the other option validations in the plugin.fallthrough) whenfallthrough: falseis used in the default pre-REST position, namingafter: 'rest'as the remedy.config-app.schema.json: the static component is now described (files, urlPath, index, extensions, fallthrough, notFound, before, after), so editors using the schema get completion and validation.Verification
unitTests/server/static.test.js(12 tests) covering the ordering options passed toserver.http, validation errors, and warning behavior — hand-rolled fake scope, plainassert, per house style.after: 'rest':GET /Greeting/→200 application/json(the resource),GET /some/deep/link→200 text/html(index.html fallback),GET /and built assets serve normally. History-mode routing and the REST API coexist.hdb.logat startup; behavior is unchanged (GET /Greeting/still returns index.html) so nothing breaks for existing pure-static apps.oxlint --deny-warningsandprettier --checkclean on the changed files.npx mocha unitTests/server/static.test.js— 12 passing. (The fulltest:unit:serversuite fails identically onmainin my local environment — pre-existing, unrelated.)Follow-up
Docs: documentation#562 (draft) adds a Handler Ordering section to the Static Files page and corrects its SPA example to use
after: 'rest'.🤖 Generated with Claude Code