Conversation
🦋 Changeset detectedLatest commit: 7488793 The changes in this PR will be included in the next version bump. 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 |
07ecc7d to
042e959
Compare
822296f to
627914d
Compare
Varixo
left a comment
There was a problem hiding this comment.
Great work! Added some comments/questions
| @@ -0,0 +1,3 @@ | |||
| '@qwik.dev/router': minor | |||
|
|
|||
| Refactor route loaders to be backed by shared async signals across SSR, client refresh, and action invalidation. | |||
| const routeFiles = node._files | ||
| .filter((f) => f.type === 'route' || f.type === 'layout') | ||
| .map((f) => f.filePath); |
There was a problem hiding this comment.
how much performance matters here?
There was a problem hiding this comment.
not much, it's build time, so a few ms won't matter
| const result = await runValidators(requestEv, action.__validators, data, devMode); | ||
| if (!result.success) { | ||
| actionError = requestEv.fail(result.status ?? 500, result.error); | ||
| } else { | ||
| const actionResolved = devMode | ||
| ? await measure(requestEv, action.__qrl.getHash(), () => | ||
| action!.__qrl.call(requestEv, result.data as JSONObject, requestEv) | ||
| ) | ||
| : await action.__qrl.call(requestEv, result.data as JSONObject, requestEv); | ||
| if (devMode) { | ||
| verifySerializable(actionResolved, action.__qrl); | ||
| } | ||
| if (actionResolved instanceof ServerError) { | ||
| actionError = actionResolved; | ||
| } else { | ||
| actionData = actionResolved; | ||
| } | ||
| } | ||
| } catch (err) { | ||
| if (err instanceof ServerError) { | ||
| actionError = err; | ||
| } else if (err instanceof Error) { | ||
| console.error('Action error:', err); | ||
| actionError = new ServerError(500, 'Internal Server Error'); | ||
| } else { | ||
| // RedirectMessage, AbortMessage, etc. — re-throw for middleware | ||
| throw err; | ||
| } | ||
| } |
There was a problem hiding this comment.
maybe wrap with something like executeAction?
| function now() { | ||
| return typeof performance !== 'undefined' ? performance.now() : 0; | ||
| } |
There was a problem hiding this comment.
this should be moved to some utils
| /** @public */ | ||
| export const routeLoader$: LoaderConstructor = /*#__PURE__*/ implicit$FirstArg(routeLoaderQrl); | ||
|
|
||
| async function runValidators( |
There was a problem hiding this comment.
similar function in action-handler.ts
| if (g._R && loaderHashes) { | ||
| loaderHashes.push(...g._R); | ||
| if (loaderPathsByHash) { | ||
| for (const hash of g._R) { |
| if (node._R && loaderHashes) { | ||
| loaderHashes.push(...node._R); | ||
| if (loaderPathsByHash) { | ||
| for (const hash of node._R) { |
There was a problem hiding this comment.
and here too. I wont make more comments about for of, maybe we should enable the eslint rule for qwik router too
| params: PathParams; | ||
| response: EndpointResponse; | ||
| loadedRoute: LoadedRoute; | ||
| routeLoaderCtx: import('./route-loaders').RouteLoaderCtx; |
There was a problem hiding this comment.
we we cant import this type normally?
| [ | ||
| { pathname: '/', expect: '/q-data.json' }, | ||
| { pathname: '/about', expect: '/about/q-data.json' }, | ||
| { pathname: '/about/', expect: '/about/q-data.json' }, | ||
| ].forEach((t) => { | ||
| test(`getClientEndpointUrl("${t.pathname}")`, () => { | ||
| const endpointPath = getClientDataPath(t.pathname); | ||
| assert.equal(endpointPath, t.expect); | ||
| }); | ||
| }); | ||
|
|
||
| [ | ||
| { pathname: '/', search: '?foo=bar', expect: '/q-data.json?foo=bar' }, | ||
| { pathname: '/about', search: '?foo=bar', expect: '/about/q-data.json?foo=bar' }, | ||
| { pathname: '/about/', search: '?foo=bar', expect: '/about/q-data.json?foo=bar' }, | ||
| { pathname: '/about/', search: '?foo=bar&baz=qux', expect: '/about/q-data.json?foo=bar&baz=qux' }, | ||
| ].forEach((t) => { | ||
| test(`getClientEndpointUrl("${t.pathname}", "${t.search}")`, () => { | ||
| const endpointPath = getClientDataPath(t.pathname, t.search); | ||
| assert.equal(endpointPath, t.expect); | ||
| }); | ||
| }); | ||
|
|
4dd423a to
d296b44
Compare
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
8f666d8 to
9cca76c
Compare
Replace the q-data.json endpoint with per-loader `q-loader-{id}.{manifestHash}.json`
endpoints. Each routeLoader$ gets its own cacheable JSON endpoint.
Key changes:
- Route trie includes loader hashes per route segment
- New loaderHandler returns individual loader data as `{d, r, e}`
- New jsonRequestWrapper captures middleware redirects/errors for JSON requests
- Action handler split into separate `handlers/action-handler.ts`
- Route loaders are AsyncSignal, tracking their route paths
- SPA navigation awaits loader promises with navCount-based redirect detection
- SSG updated for per-loader endpoints
- Core: export additional internals needed by router spec tests
Add jsonRequestWrapper handler that wraps next() in try/catch for
q-loader and q-action JSON requests. Middleware redirects/errors are
captured into JSON envelopes ({r} for loaders, {e,s} for actions)
instead of propagating as HTTP redirects/error pages.
- Loader redirects: returned as {r: url} in LoaderResponse envelope
- Action redirects: re-thrown (client handles via response.redirected)
- Errors: wrapped as ServerError in both loader and action envelopes
- Dev mode: error messages include original error text
- SSR loadersMiddleware: unchanged, errors propagate for middleware
routeLoader$ now accepts an `eTag` option for ETag-based caching of q-loader-*.json responses: - `eTag: true` — auto-hash serialized data (loader runs, then checks) - `eTag: "version"` — static eTag (304 returned before loader runs) - `eTag: (ev) => string|null` — dynamic from request context (params, URL, etc.), 304 returned before loader runs For string/function eTags, the loader is skipped entirely on cache hit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
routeLoader$ now accepts a `search` option — an allowlist of URL search
parameter names the loader depends on.
When set:
- Only re-fetches when the listed search params change
- Other param changes are ignored (returns previous value)
- Only the listed params are sent in the loader JSON request URL
When not set: all params sent, any change triggers re-fetch (current behavior).
Example: `routeLoader$(fn, { search: ['sort', 'page'] })`
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use page.waitForURL() after SPA navigation clicks before asserting
content. URL changes complete before loader data renders, providing
a stable synchronization point.
- Assert loader-dependent values first (they change between routes)
before checking static values (same title on both routes).
- Increase timeout for loader redirect test (multi-step: fetch →
{r} envelope → goto).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ce335be to
3e44f25
Compare
Add strictLoaders Vite plugin option (default: true) that makes loaders default to search:[] and actions default to invalidate:[]. This maximizes cacheability. Add allowStale option to LoaderOptions, passed through to the AsyncSignal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
node 24 compat
When variable migration collected transitive dependencies for a root variable being migrated to a single segment, it didn't check whether those transitive deps were also directly used by other segments. This caused the deps to be migrated (inlined) into one segment and their exports removed from the parent module, leaving other segments with dangling references. In qwik-router, this manifested as `currentScrollState` and `saveScrollHistory` being migrated to the useTask segment (as transitive deps of other helpers) while the goto segment also needed them, causing ReferenceErrors at runtime during SPA navigation. The fix adds a check in the safety filter of `find_migratable_vars`: if a candidate variable appears in `root_var_usage` for any segment other than the migration target, it is excluded from migration.
it's a bit of a workaround but it's server-side only and it is needed for robustness against bundling issues
Preserve AsyncSignal subscriptions across a loader-driven redirect by returning `previous` instead of throwing — an error-state AsyncSignal can drop its Resource subscription, which would prevent the redirect-target fetch from updating the UI. The UX trade-off (a brief flash of stale data before the new route's loaders resolve) is intentional; awaiting every loader promise on every nav to catch the rare redirect case is too costly. - Gate the welcome-redirect assertions in loaders.e2e.ts on MPA only, since SPA races the commit against the redirect nav and doesn't guarantee the composed loader re-resolves with fresh data. - Refresh the SSG snapshot fixtures for the new serialized state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3e44f25 to
7488793
Compare
opening for visibility