From 58969513cad319e702e9b15427cfc8c8c74f031f Mon Sep 17 00:00:00 2001 From: Jeff Repanich Date: Fri, 9 Oct 2026 13:28:21 -0400 Subject: [PATCH] refactor(router): separate authoring policy and record registration --- CHANGELOG.md | 4 + docs/development/0.5.0-readiness.md | 8 +- docs/internals/route-authoring-ownership.md | 31 ++ src/router/authoring.ts | 423 +----------------- src/router/path-policy.ts | 98 ++++ src/router/registration-scope.ts | 104 +++++ src/router/route-registration.ts | 243 ++++++++++ tests/checks/architecture.test.ts | 72 +++ .../authoring-registration-policy.test.ts | 79 ++++ .../router/authoring-scope-ownership.test.ts | 193 ++++++++ 10 files changed, 842 insertions(+), 413 deletions(-) create mode 100644 docs/internals/route-authoring-ownership.md create mode 100644 src/router/path-policy.ts create mode 100644 src/router/registration-scope.ts create mode 100644 src/router/route-registration.ts create mode 100644 tests/unit/router/authoring-registration-policy.test.ts create mode 100644 tests/unit/router/authoring-scope-ownership.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 8af52466..291a044b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ ### Fixes +- Enforce startup locks and component-function checks for index and fallback + routes, retain a page's index slot after failed registration, and normalize + root-page fallback paths to `/*`. + - Ignore query results and consistency errors superseded by their own callbacks; preserve newer invalidation work created by start or reconciliation abort listeners instead of starting obsolete transport or suppressing retries. diff --git a/docs/development/0.5.0-readiness.md b/docs/development/0.5.0-readiness.md index 554c1891..03b97469 100644 --- a/docs/development/0.5.0-readiness.md +++ b/docs/development/0.5.0-readiness.md @@ -2,9 +2,9 @@ Review date: 2026-10-09. This is a dated planning and evidence snapshot, not a release qualification certificate. -The initial organization inventory contained 24 repositories and 61 open issues. The existing 0.5.0 milestones contain 48 issues. The maintainer selected **existing milestone plus confirmed bugs**: include five additional confirmed CLI/Themes defects, for 53 issues in the working release scope. All five confirmed bugs, the Logos, Lucide, Monaco, telemetry and i18n hardening gates, and the SSR output, DOM node and host prop refactors are now closed; 40 scoped issues remain, plus eight backlog requests. Keep eight newer feature/integration requests in the backlog. No existing milestone issue is deferred by this review. +The initial organization inventory contained 24 repositories and 61 open issues. The existing 0.5.0 milestones contain 48 issues. The maintainer selected **existing milestone plus confirmed bugs**: include five additional confirmed CLI/Themes defects, for 53 issues in the working release scope. All five confirmed bugs, the Logos, Lucide, Monaco, telemetry and i18n hardening gates, and the SSR output, DOM node, host prop and query lifetime refactors are now closed; 39 scoped issues remain, plus eight backlog requests. Keep eight newer feature/integration requests in the backlog. No existing milestone issue is deferred by this review. -**Readiness: blocked.** The API contraction, deeper hardening evidence, three remaining core boundary refactors, coverage baseline, packed consumer matrix, and exact-version qualification remain open. Every published package is still on 0.4.x. An existing test suite or issue title does not establish the required hardening outcome. +**Readiness: blocked.** The API contraction, deeper hardening evidence, two remaining core boundary refactors, coverage baseline, packed consumer matrix, and exact-version qualification remain open. Every published package is still on 0.4.x. An existing test suite or issue title does not establish the required hardening outcome. ## Delivery order @@ -50,7 +50,7 @@ Every issue in the initial inventory is included below; delivered rows retain th | [askr#737](https://github.com/askrjs/askr/issues/737): refactor(ssr): isolate buffered output and portal finalization | Closed | Closed in core PR #761; atomic output, portals, references and hydration retain their contracts. | | [askr#738](https://github.com/askrjs/askr/issues/738): refactor(renderer): split node dispatch from hydration and lifecycle coordination | Closed | PR #762 merged after focused/full checks, two measured workloads, architecture guards and cursor-failure retry regression. | | [askr#739](https://github.com/askrjs/askr/issues/739): refactor(renderer): separate host prop lifecycle from prop value writers | Closed | PR #763 merged after mixed lifetime rollback/retry probes, full checks and production artifact comparisons. | -| [askr#740](https://github.com/askrjs/askr/issues/740): refactor(data): make QueryCell ownership and async state boundaries explicit | Release | Extract the private boundary with unchanged behavior; characterize the seam and pass required tests. | +| [askr#740](https://github.com/askrjs/askr/issues/740): refactor(data): make QueryCell ownership and async state boundaries explicit | Closed | PR #764 merged after six original-source regression failures, transition probes, full checks and three cache measurements. | | [askr#741](https://github.com/askrjs/askr/issues/741): refactor(router): separate route scopes and path policy from record registration | Release | Extract the private boundary with unchanged behavior; characterize the seam and pass required tests. | | [askr#742](https://github.com/askrjs/askr/issues/742): refactor(router): isolate navigation resolution from root and history commits | Release | Extract the private boundary with unchanged behavior; characterize the seam and pass required tests. | | [askr#743](https://github.com/askrjs/askr/issues/743): test: measure branch coverage for critical runtime transitions | Release | Measure supported environments; set measured per-area branch floors; retain all existing suites. | @@ -160,3 +160,5 @@ GitHub issues and comments were fetched per repository, rather than relying on s Before #744 is closed, attach: package issue closures or approved scope changes; symbol/path removal and migration commits; hardening scenario outcomes; measured coverage; exact full-check results; all 18 package versions/commit IDs/tarball digests; clean packed consumer results; hosted promotion checks; tag targets; registry integrity/provenance; and clean post-publish consumer readback. Use [Coordinated release](./coordinated-release.md) for wave sequencing and [Release](./release.md) for package/promotion gates. + +- Core query lifetime PR [#764](https://github.com/askrjs/askr/pull/764) merged as `e31d344552471fd9616b331dca6aa845750b1938` and closed #740. Reader/definition ownership, retention timers and cache retirement now live below async publication. Six reproduced failures cover callback invalidation, abort-listener work, reconciliation retries and immediate-GC reattachment. Complete qualification passed 439 unit, 94 repository, 2,695 jsdom, 158 browser and 12 installed type cases. Three before/after cache-dedupe/abort captures retained all assertions with a 1.7% median increase. Exact-head and merge-head CI/CodeQL passed. diff --git a/docs/internals/route-authoring-ownership.md b/docs/internals/route-authoring-ownership.md new file mode 100644 index 00000000..03089caf --- /dev/null +++ b/docs/internals/route-authoring-ownership.md @@ -0,0 +1,31 @@ +# Route authoring ownership + +`router/authoring.ts` retains the public overloads, component/path type inference, +execution-model restrictions and typed route references. Its implementations +delegate to three private modules; none is a package entrypoint. + +| Owner | Responsibility | +| ----------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `path-policy.ts` | Validate parameter syntax and normalize/join paths without consulting a registration table. | +| `registration-scope.ts` | Resolve paths against the active scope and capture group/page inheritance. Scope push/pop remains owned by the table in `store.ts`. | +| `route-registration.ts` | Validate record eligibility, compose inherited auth/policies/metadata, reject duplicate match keys, construct handlers and publish records into the active table. | + +Scope callbacks remain synchronous. A throwing definition unwinds every scope +through `store.ts`'s `finally` blocks. Records registered before that exception +remain registered; route definitions are not transactions. A later root route +must not inherit the failed definition's path, layout, page, auth or metadata. + +All leaf registration helpers (`route`, `index`, `fallback`) must respect the +active table's startup lock and accept only component functions. A fresh +`createRouteRegistry` uses its own unlocked table. `index` reserves its page's +single index slot only after the record is successfully registered. + +Fallback paths use the same join rules as relative leaf paths. In particular, +`page('/', Page, ...)` owns `/*`, with `/` as its fallback prefix and `Page` in +its page chain. Nested-group fallback restrictions and inherited auth denial +order are unchanged. + +The architecture tests enforce the dependency direction: path policy cannot +read the table; scope capture cannot publish records; record registration cannot +import the public facade. Unit probes exercise these seams alongside the +existing manifest, matching, auth, metadata and jsdom navigation suites. diff --git a/src/router/authoring.ts b/src/router/authoring.ts index 4b5fe7af..ba140ccc 100644 --- a/src/router/authoring.ts +++ b/src/router/authoring.ts @@ -1,6 +1,5 @@ import type { GroupHelperOptions, - ParsedSegment, PageHelperOptions, RouteComponent, RouteDefinition, @@ -10,36 +9,24 @@ import type { RouteRef, RouteRefSearch, } from '../common/router'; -import type { AuthDecision, AuthRequirement } from '@askrjs/auth'; import type { ObjectSchema } from '@askrjs/schema'; import { currentComponent as getCurrentComponentInstance } from '../core/api/hooks'; import { getExecutionModel } from '../common/execution-model'; +import type { AnyRouteComponent } from './internal-types'; import { - computeRank, - normalizeRouteSegmentName, - parseSegments, - routeMatchKey, -} from './match'; -import { compileNodePolicies } from './access'; -import type { AnyRouteComponent, InternalRouteRecord } from './internal-types'; -import { createRouteHandler } from './rendering'; -import { - addRouteToStores, assertRouteRegistrationUnlocked, - getCurrentInheritedAuthRequirements, - getCurrentInheritedMeta, - getCurrentInheritedPolicies, - getCurrentLayoutChain, - getCurrentPageChain, - getCurrentPageScope, - getCurrentPathPrefix, - getCurrentScopeKind, getDefaultRouteBasePath, - getRouteRecords, - hasActivePageScope, - insertRecordSorted, - pushRegistrationScope, } from './store'; +import { + pushGroupScope, + pushPageScope, + resolveRouteRegistrationPath, +} from './registration-scope'; +import { + registerRouteAtResolvedPath, + registerIndexRoute, + registerFallbackRoute, +} from './route-registration'; type RouteComponentParam = Parameters extends [] ? unknown : Parameters[0]; @@ -106,351 +93,6 @@ type RouteOptionsForComponent< TDehydratedData >; -function validateRoutePath(path: string): void { - if (!path.startsWith('/')) { - throw new Error(`Route path must begin with "/". Got: "${path}"`); - } - if (/\/{2,}/.test(path)) { - throw new Error('Route path cannot contain consecutive slashes.'); - } - if (/:([^/{}]+)/.test(path)) { - const suggested = path.replace(/:([^/{}]+)/g, '{$1}'); - throw new Error( - `Route parameter syntax uses {name} interpolation, not :name. ` + - `Use "${suggested}" instead of "${path}".` - ); - } - - const segments = path.split('/').filter(Boolean); - const seenParamNames = new Set(); - - for (let index = 0; index < segments.length; index++) { - const segment = segments[index]; - if (segment === '*') { - continue; - } - - const hasOpenBrace = segment.includes('{'); - const hasCloseBrace = segment.includes('}'); - - if (!hasOpenBrace && !hasCloseBrace) { - continue; - } - - if (!(segment.startsWith('{') && segment.endsWith('}'))) { - throw new Error( - 'Route parameter segments must use complete {name} interpolation.' - ); - } - - const rawParamName = normalizeRouteSegmentName(segment.slice(1, -1)); - const isSplat = rawParamName.startsWith('*'); - const paramName = isSplat - ? normalizeRouteSegmentName(rawParamName.slice(1)) - : rawParamName; - - if (!paramName) { - throw new Error( - isSplat - ? 'Route splat parameter name cannot be empty.' - : 'Route parameter name cannot be empty.' - ); - } - - if (isSplat && paramName === '*') { - throw new Error('Route named splat parameter name cannot be "*".'); - } - - if (isSplat && index !== segments.length - 1) { - throw new Error( - 'Route named splat parameters must be the final segment.' - ); - } - - if (seenParamNames.has(paramName)) { - throw new Error( - `Route path cannot reuse duplicate parameter name "${paramName}".` - ); - } - - seenParamNames.add(paramName); - } -} - -function normalizeAbsoluteRoutePath(path: string): string { - if (!path || path === '/') { - return '/'; - } - - const normalized = path.endsWith('/') ? path.slice(0, -1) : path; - return normalized || '/'; -} - -function joinRoutePaths(prefix: string, path: string): string { - const normalizedPrefix = normalizeAbsoluteRoutePath(prefix || '/'); - let start = 0; - let end = path.length; - while (start < end && path[start] === '/') start++; - while (end > start && path[end - 1] === '/') end--; - const normalizedPath = path.slice(start, end); - - if (!normalizedPath) { - return normalizedPrefix; - } - - return normalizedPrefix === '/' - ? `/${normalizedPath}` - : `${normalizedPrefix}/${normalizedPath}`; -} - -function resolvePageScopePath(path: string): string { - if (!path) { - throw new Error('page(path, Component, fn) requires a non-empty path.'); - } - - if (path.startsWith('/')) { - validateRoutePath(path); - return normalizeAbsoluteRoutePath(path); - } - - return joinRoutePaths(getCurrentPathPrefix(), path); -} - -function resolveIndexPath(): string { - return normalizeAbsoluteRoutePath(getCurrentPathPrefix() || '/'); -} - -function resolveRouteRegistrationPath(path: string): string { - if (path.startsWith('/')) { - if (hasActivePageScope()) { - throw new Error( - 'Child route paths inside page() must be relative. ' + - `Use "${path.slice(1)}" instead of "${path}".` - ); - } - - validateRoutePath(path); - return normalizeAbsoluteRoutePath(path); - } - - const prefix = getCurrentPathPrefix(); - - if (!prefix) { - throw new Error(`Route path must begin with "/". Got: "${path}"`); - } - - return joinRoutePaths(prefix, path); -} - -function pushGroupScope( - options: GroupHelperOptions, - fn: RouteDefinition -): void { - const policies = compileNodePolicies(options); - - pushRegistrationScope( - { - kind: 'group', - pathPrefix: getCurrentPathPrefix(), - layout: options.layout, - auth: options.auth, - policies, - meta: options.meta, - }, - fn - ); -} - -function pushPageScope( - path: string, - Component: RouteComponent, - options: PageHelperOptions, - fn: RouteDefinition -): void { - if (hasActivePageScope()) { - throw new Error( - 'page() cannot be nested inside another page(). ' + - 'Use route() for child leaves or group() for inherited behavior inside the existing page scope.' - ); - } - - const policies = compileNodePolicies(options); - - pushRegistrationScope( - { - kind: 'page', - pathPrefix: resolvePageScopePath(path), - page: Component, - hasIndex: false, - auth: options.auth, - policies, - meta: options.meta, - }, - fn - ); -} - -function normalizeRouteOptions( - options: RouteOptions | undefined -): RouteOptions | undefined { - if (!options) { - return undefined; - } - - const loader = options.loader; - const dehydrate = options.dehydrate; - const preload = options.preload; - const policies = compileNodePolicies(options); - - if ( - !loader && - !dehydrate && - !preload && - !options.entries && - !options.invalidationKeys && - policies.length === 0 && - !options.title && - !options.namespace && - !options.search && - !options.meta && - !options.actions && - options.auth === undefined - ) { - return undefined; - } - - return { - ...(loader ? { loader } : {}), - ...(dehydrate ? { dehydrate } : {}), - ...(preload ? { preload } : {}), - ...(options.entries ? { entries: options.entries } : {}), - ...(options.invalidationKeys - ? { invalidationKeys: options.invalidationKeys } - : {}), - ...(options.auth !== undefined ? { auth: options.auth } : {}), - ...(policies.length > 0 ? { policies } : {}), - ...(options.title ? { title: options.title } : {}), - ...(options.namespace ? { namespace: options.namespace } : {}), - ...(options.search ? { search: options.search } : {}), - ...(options.meta ? { meta: options.meta } : {}), - ...(options.actions ? { actions: options.actions } : {}), - }; -} - -function assertRouteNotDuplicated( - path: string, - segments: ParsedSegment[], - fallbackPrefix: string | undefined -): void { - const key = routeMatchKey(segments, fallbackPrefix); - const existing = getRouteRecords().find( - (record) => routeMatchKey(record.segments, record.fallbackPrefix) === key - ); - if (existing) { - const hint = - existing.path === path - ? " To generate several pages from one route template, declare it once and return each page's params from entries()." - : ''; - throw new Error( - `Duplicate route path "${path}": it matches the same URLs as "${existing.path}", which is already registered.${hint}` - ); - } -} - -const authAllowed: AuthDecision = Object.freeze({ allowed: true }); - -// Inherited and route requirements must all allow, evaluated in declaration -// order; the first denial wins. Kept local so @askrjs/auth stays type-only. -function requireAll(requirements: AuthRequirement[]): AuthRequirement { - return async (context) => { - for (const requirement of requirements) { - const decision = await requirement(context); - if (!decision.allowed) return decision; - } - return authAllowed; - }; -} - -function registerRouteAtResolvedPath( - path: string, - Component: RouteComponent, - options?: RouteOptions, - metadata?: { - isFallback?: boolean; - fallbackPrefix?: string; - } -): void { - validateRoutePath(path); - - const segments = parseSegments(path); - assertRouteNotDuplicated(path, segments, metadata?.fallbackPrefix); - - const chain = getCurrentLayoutChain(); - const pageChain = getCurrentPageChain(); - const rank = computeRank(segments); - const isFallback = metadata?.isFallback ?? path === '/*'; - const comp = Component; - const normalizedOptions = normalizeRouteOptions(options); - const policies = [ - ...getCurrentInheritedPolicies(), - ...(normalizedOptions?.policies ?? []), - ]; - const authRequirements = [ - ...getCurrentInheritedAuthRequirements(), - ...(normalizedOptions?.auth ? [normalizedOptions.auth] : []), - ]; - const auth: AuthRequirement | undefined = - authRequirements.length === 0 - ? undefined - : authRequirements.length === 1 - ? authRequirements[0] - : requireAll(authRequirements); - const metaChain = [ - ...getCurrentInheritedMeta(), - ...(normalizedOptions?.meta ? [normalizedOptions.meta] : []), - ]; - - const handler = createRouteHandler(comp, pageChain, chain); - const renderHandler = createRouteHandler(comp, pageChain, chain, true); - - const record: InternalRouteRecord = { - path, - component: comp, - segments, - rank, - layoutChain: chain, - pageChain, - options: normalizedOptions - ? { - ...normalizedOptions, - ...(auth ? { auth } : {}), - ...(policies.length > 0 ? { policies } : {}), - } - : policies.length > 0 - ? { policies, ...(auth ? { auth } : {}) } - : auth - ? { auth } - : {}, - ...(metaChain.length > 0 ? { metaChain } : {}), - isFallback, - handler, - renderHandler, - ...(metadata?.fallbackPrefix - ? { fallbackPrefix: metadata.fallbackPrefix } - : {}), - }; - - insertRecordSorted(record); - addRouteToStores({ - path, - handler, - namespace: normalizedOptions?.namespace ?? options?.namespace, - ...(metadata?.fallbackPrefix - ? { fallbackPrefix: metadata.fallbackPrefix } - : {}), - }); -} - /** Declare a group of routes sharing `options` (auth, policies, layout, meta). */ export function group(options: GroupHelperOptions, fn: RouteDefinition): void; export function group(options: GroupHelperOptions, fn: RouteDefinition): void { @@ -516,51 +158,12 @@ export function page( /** Declare the index route for the enclosing `page()` scope. */ export function index(Component: RouteComponent, options?: RouteOptions): void { - const pageScope = getCurrentPageScope(); - if (pageScope?.hasIndex) { - throw new Error('page() cannot declare multiple index routes.'); - } - - if (pageScope) { - pageScope.hasIndex = true; - } - - registerRouteAtResolvedPath(resolveIndexPath(), Component, options); + registerIndexRoute(Component, options); } /** Declare the catch-all `/*` fallback route for the enclosing scope. */ export function fallback(Component: RouteComponent): void { - if (hasActivePageScope()) { - if (getCurrentScopeKind() !== 'page') { - throw new Error( - 'fallback() inside page() must be declared directly in the page scope, not inside nested group().' - ); - } - - registerRouteAtResolvedPath( - `${getCurrentPathPrefix()}/*`, - Component, - undefined, - { isFallback: true, fallbackPrefix: getCurrentPathPrefix() } - ); - return; - } - - const allowsRootFallback = - getCurrentInheritedPolicies().length === 0 && - getCurrentInheritedAuthRequirements().length === 0; - - if (!allowsRootFallback) { - throw new Error( - 'fallback() can only be registered at the root scope. ' + - 'Use route("/*", Component) if you need compatibility behavior.' - ); - } - - registerRouteAtResolvedPath('/*', Component, undefined, { - isFallback: true, - fallbackPrefix: '/', - }); + registerFallbackRoute(Component); } /** Declare a route at `path` rendering `Component`, returning a typed {@link RouteRef} for building destinations. */ diff --git a/src/router/path-policy.ts b/src/router/path-policy.ts new file mode 100644 index 00000000..837a8a58 --- /dev/null +++ b/src/router/path-policy.ts @@ -0,0 +1,98 @@ +import { normalizeRouteSegmentName } from './match'; + +export function validateRoutePath(path: string): void { + if (!path.startsWith('/')) { + throw new Error(`Route path must begin with "/". Got: "${path}"`); + } + if (/\/{2,}/.test(path)) { + throw new Error('Route path cannot contain consecutive slashes.'); + } + if (/:([^/{}]+)/.test(path)) { + const suggested = path.replace(/:([^/{}]+)/g, '{$1}'); + throw new Error( + `Route parameter syntax uses {name} interpolation, not :name. ` + + `Use "${suggested}" instead of "${path}".` + ); + } + + const segments = path.split('/').filter(Boolean); + const seenParamNames = new Set(); + + for (let index = 0; index < segments.length; index++) { + const segment = segments[index]; + if (segment === '*') { + continue; + } + + const hasOpenBrace = segment.includes('{'); + const hasCloseBrace = segment.includes('}'); + + if (!hasOpenBrace && !hasCloseBrace) { + continue; + } + + if (!(segment.startsWith('{') && segment.endsWith('}'))) { + throw new Error( + 'Route parameter segments must use complete {name} interpolation.' + ); + } + + const rawParamName = normalizeRouteSegmentName(segment.slice(1, -1)); + const isSplat = rawParamName.startsWith('*'); + const paramName = isSplat + ? normalizeRouteSegmentName(rawParamName.slice(1)) + : rawParamName; + + if (!paramName) { + throw new Error( + isSplat + ? 'Route splat parameter name cannot be empty.' + : 'Route parameter name cannot be empty.' + ); + } + + if (isSplat && paramName === '*') { + throw new Error('Route named splat parameter name cannot be "*".'); + } + + if (isSplat && index !== segments.length - 1) { + throw new Error( + 'Route named splat parameters must be the final segment.' + ); + } + + if (seenParamNames.has(paramName)) { + throw new Error( + `Route path cannot reuse duplicate parameter name "${paramName}".` + ); + } + + seenParamNames.add(paramName); + } +} + +export function normalizeAbsoluteRoutePath(path: string): string { + if (!path || path === '/') { + return '/'; + } + + const normalized = path.endsWith('/') ? path.slice(0, -1) : path; + return normalized || '/'; +} + +export function joinRoutePaths(prefix: string, path: string): string { + const normalizedPrefix = normalizeAbsoluteRoutePath(prefix || '/'); + let start = 0; + let end = path.length; + while (start < end && path[start] === '/') start++; + while (end > start && path[end - 1] === '/') end--; + const normalizedPath = path.slice(start, end); + + if (!normalizedPath) { + return normalizedPrefix; + } + + return normalizedPrefix === '/' + ? `/${normalizedPath}` + : `${normalizedPrefix}/${normalizedPath}`; +} diff --git a/src/router/registration-scope.ts b/src/router/registration-scope.ts new file mode 100644 index 00000000..7eb9665b --- /dev/null +++ b/src/router/registration-scope.ts @@ -0,0 +1,104 @@ +import type { + GroupHelperOptions, + PageHelperOptions, + RouteComponent, + RouteDefinition, +} from '../common/router'; +import { compileNodePolicies } from './access'; +import { + hasActivePageScope, + getCurrentPathPrefix, + pushRegistrationScope, +} from './store'; +import { + validateRoutePath, + normalizeAbsoluteRoutePath, + joinRoutePaths, +} from './path-policy'; + +function resolvePageScopePath(path: string): string { + if (!path) { + throw new Error('page(path, Component, fn) requires a non-empty path.'); + } + + if (path.startsWith('/')) { + validateRoutePath(path); + return normalizeAbsoluteRoutePath(path); + } + + return joinRoutePaths(getCurrentPathPrefix(), path); +} + +export function resolveIndexPath(): string { + return normalizeAbsoluteRoutePath(getCurrentPathPrefix() || '/'); +} + +export function resolveRouteRegistrationPath(path: string): string { + if (path.startsWith('/')) { + if (hasActivePageScope()) { + throw new Error( + 'Child route paths inside page() must be relative. ' + + `Use "${path.slice(1)}" instead of "${path}".` + ); + } + + validateRoutePath(path); + return normalizeAbsoluteRoutePath(path); + } + + const prefix = getCurrentPathPrefix(); + + if (!prefix) { + throw new Error(`Route path must begin with "/". Got: "${path}"`); + } + + return joinRoutePaths(prefix, path); +} + +export function pushGroupScope( + options: GroupHelperOptions, + fn: RouteDefinition +): void { + const policies = compileNodePolicies(options); + + pushRegistrationScope( + { + kind: 'group', + pathPrefix: getCurrentPathPrefix(), + layout: options.layout, + auth: options.auth, + policies, + meta: options.meta, + }, + fn + ); +} + +export function pushPageScope( + path: string, + Component: RouteComponent, + options: PageHelperOptions, + fn: RouteDefinition +): void { + if (hasActivePageScope()) { + throw new Error( + 'page() cannot be nested inside another page(). ' + + 'Use route() for child leaves or group() for inherited behavior inside the existing page scope.' + ); + } + + const policies = compileNodePolicies(options); + + pushRegistrationScope( + { + kind: 'page', + pathPrefix: resolvePageScopePath(path), + page: Component, + hasIndex: false, + auth: options.auth, + policies, + meta: options.meta, + }, + fn + ); +} diff --git a/src/router/route-registration.ts b/src/router/route-registration.ts new file mode 100644 index 00000000..8652731f --- /dev/null +++ b/src/router/route-registration.ts @@ -0,0 +1,243 @@ +import type { + ParsedSegment, + RouteComponent, + RouteOptions, +} from '../common/router'; +import type { AuthDecision, AuthRequirement } from '@askrjs/auth'; +import type { InternalRouteRecord } from './internal-types'; +import { computeRank, parseSegments, routeMatchKey } from './match'; +import { compileNodePolicies } from './access'; +import { createRouteHandler } from './rendering'; +import { joinRoutePaths, validateRoutePath } from './path-policy'; +import { resolveIndexPath } from './registration-scope'; +import { + addRouteToStores, + assertRouteRegistrationUnlocked, + getCurrentInheritedAuthRequirements, + getCurrentInheritedMeta, + getCurrentInheritedPolicies, + getCurrentLayoutChain, + getCurrentPageChain, + getCurrentPageScope, + getCurrentPathPrefix, + getCurrentScopeKind, + getRouteRecords, + hasActivePageScope, + insertRecordSorted, +} from './store'; + +function normalizeRouteOptions( + options: RouteOptions | undefined +): RouteOptions | undefined { + if (!options) { + return undefined; + } + + const loader = options.loader; + const dehydrate = options.dehydrate; + const preload = options.preload; + const policies = compileNodePolicies(options); + + if ( + !loader && + !dehydrate && + !preload && + !options.entries && + !options.invalidationKeys && + policies.length === 0 && + !options.title && + !options.namespace && + !options.search && + !options.meta && + !options.actions && + options.auth === undefined + ) { + return undefined; + } + + return { + ...(loader ? { loader } : {}), + ...(dehydrate ? { dehydrate } : {}), + ...(preload ? { preload } : {}), + ...(options.entries ? { entries: options.entries } : {}), + ...(options.invalidationKeys + ? { invalidationKeys: options.invalidationKeys } + : {}), + ...(options.auth !== undefined ? { auth: options.auth } : {}), + ...(policies.length > 0 ? { policies } : {}), + ...(options.title ? { title: options.title } : {}), + ...(options.namespace ? { namespace: options.namespace } : {}), + ...(options.search ? { search: options.search } : {}), + ...(options.meta ? { meta: options.meta } : {}), + ...(options.actions ? { actions: options.actions } : {}), + }; +} + +function assertRouteNotDuplicated( + path: string, + segments: ParsedSegment[], + fallbackPrefix: string | undefined +): void { + const key = routeMatchKey(segments, fallbackPrefix); + const existing = getRouteRecords().find( + (record) => routeMatchKey(record.segments, record.fallbackPrefix) === key + ); + if (existing) { + const hint = + existing.path === path + ? " To generate several pages from one route template, declare it once and return each page's params from entries()." + : ''; + throw new Error( + `Duplicate route path "${path}": it matches the same URLs as "${existing.path}", which is already registered.${hint}` + ); + } +} + +const authAllowed: AuthDecision = Object.freeze({ allowed: true }); + +// Inherited and route requirements must all allow, evaluated in declaration +// order; the first denial wins. Kept local so @askrjs/auth stays type-only. +function requireAll(requirements: AuthRequirement[]): AuthRequirement { + return async (context) => { + for (const requirement of requirements) { + const decision = await requirement(context); + if (!decision.allowed) return decision; + } + return authAllowed; + }; +} + +export function registerRouteAtResolvedPath( + path: string, + Component: RouteComponent, + options?: RouteOptions, + metadata?: { + isFallback?: boolean; + fallbackPrefix?: string; + } +): void { + assertRouteRegistrationUnlocked(); + if (typeof Component !== 'function') { + throw new Error('Route registration requires a component function.'); + } + validateRoutePath(path); + + const segments = parseSegments(path); + assertRouteNotDuplicated(path, segments, metadata?.fallbackPrefix); + + const chain = getCurrentLayoutChain(); + const pageChain = getCurrentPageChain(); + const rank = computeRank(segments); + const isFallback = metadata?.isFallback ?? path === '/*'; + const comp = Component; + const normalizedOptions = normalizeRouteOptions(options); + const policies = [ + ...getCurrentInheritedPolicies(), + ...(normalizedOptions?.policies ?? []), + ]; + const authRequirements = [ + ...getCurrentInheritedAuthRequirements(), + ...(normalizedOptions?.auth ? [normalizedOptions.auth] : []), + ]; + const auth: AuthRequirement | undefined = + authRequirements.length === 0 + ? undefined + : authRequirements.length === 1 + ? authRequirements[0] + : requireAll(authRequirements); + const metaChain = [ + ...getCurrentInheritedMeta(), + ...(normalizedOptions?.meta ? [normalizedOptions.meta] : []), + ]; + + const handler = createRouteHandler(comp, pageChain, chain); + const renderHandler = createRouteHandler(comp, pageChain, chain, true); + + const record: InternalRouteRecord = { + path, + component: comp, + segments, + rank, + layoutChain: chain, + pageChain, + options: normalizedOptions + ? { + ...normalizedOptions, + ...(auth ? { auth } : {}), + ...(policies.length > 0 ? { policies } : {}), + } + : policies.length > 0 + ? { policies, ...(auth ? { auth } : {}) } + : auth + ? { auth } + : {}, + ...(metaChain.length > 0 ? { metaChain } : {}), + isFallback, + handler, + renderHandler, + ...(metadata?.fallbackPrefix + ? { fallbackPrefix: metadata.fallbackPrefix } + : {}), + }; + + insertRecordSorted(record); + addRouteToStores({ + path, + handler, + namespace: normalizedOptions?.namespace ?? options?.namespace, + ...(metadata?.fallbackPrefix + ? { fallbackPrefix: metadata.fallbackPrefix } + : {}), + }); +} + +/** Declare the index route for the enclosing `page()` scope. */ +export function registerIndexRoute( + Component: RouteComponent, + options?: RouteOptions +): void { + const pageScope = getCurrentPageScope(); + if (pageScope?.hasIndex) { + throw new Error('page() cannot declare multiple index routes.'); + } + + registerRouteAtResolvedPath(resolveIndexPath(), Component, options); + if (pageScope) { + pageScope.hasIndex = true; + } +} + +/** Declare the catch-all `/*` fallback route for the enclosing scope. */ +export function registerFallbackRoute(Component: RouteComponent): void { + if (hasActivePageScope()) { + if (getCurrentScopeKind() !== 'page') { + throw new Error( + 'fallback() inside page() must be declared directly in the page scope, not inside nested group().' + ); + } + + registerRouteAtResolvedPath( + joinRoutePaths(getCurrentPathPrefix(), '*'), + Component, + undefined, + { isFallback: true, fallbackPrefix: getCurrentPathPrefix() } + ); + return; + } + + const allowsRootFallback = + getCurrentInheritedPolicies().length === 0 && + getCurrentInheritedAuthRequirements().length === 0; + + if (!allowsRootFallback) { + throw new Error( + 'fallback() can only be registered at the root scope. ' + + 'Use route("/*", Component) if you need compatibility behavior.' + ); + } + + registerRouteAtResolvedPath('/*', Component, undefined, { + isFallback: true, + fallbackPrefix: '/', + }); +} diff --git a/tests/checks/architecture.test.ts b/tests/checks/architecture.test.ts index a291a0f3..c5d4c228 100644 --- a/tests/checks/architecture.test.ts +++ b/tests/checks/architecture.test.ts @@ -240,6 +240,30 @@ function violatesQueryLifetimeBoundary(edge: Edge): boolean { ); } +function violatesRouteAuthoringBoundary(edge: Edge): boolean { + if (edge.typeOnly) return false; + const from = relative(edge.from); + const to = relative(edge.to); + if (from === 'src/router/path-policy.ts') { + return to !== 'src/router/match.ts'; + } + if (from === 'src/router/registration-scope.ts') { + return ![ + 'src/router/store.ts', + 'src/router/access.ts', + 'src/router/path-policy.ts', + ].includes(to); + } + return ( + from === 'src/router/route-registration.ts' && + [ + 'src/router/authoring.ts', + 'src/router/route.ts', + 'src/router/index.ts', + ].includes(to) + ); +} + function findCycles(): string[] { const graph = new Map>(); for (const edge of edges.filter((edge) => !edge.typeOnly)) { @@ -311,6 +335,54 @@ function findModuleCycles(): string[][] { } describe('architecture boundaries', () => { + it('should keep path policy and scope capture below record registration and public authoring', () => { + expect(edges.filter(violatesRouteAuthoringBoundary).map(format)).toEqual( + [] + ); + for (const module of ['registration-scope', 'route-registration']) { + expect( + edges.some( + (edge) => + relative(edge.from) === 'src/router/authoring.ts' && + relative(edge.to) === `src/router/${module}.ts` && + !edge.typeOnly + ) + ).toBe(true); + } + }); + + it('should reject path and scope dependencies on registration through imports, reexports and dynamic imports', () => { + const pathFile = path.join(srcDir, 'router', 'path-policy.ts'); + const scopeFile = path.join(srcDir, 'router', 'registration-scope.ts'); + const parse = (file: string, source: string) => + collectEdges( + file, + ts.createSourceFile(file, source, ts.ScriptTarget.Latest, true) + ); + const synthetic = [ + ...parse( + pathFile, + `import './store'; export * from './authoring'; import './match'; import type { RouteParams } from '../common/router';` + ), + ...parse( + scopeFile, + `const registration = import('./route-registration'); import './path-policy'; import './store';` + ), + ...parse( + path.join(srcDir, 'router', 'route-registration.ts'), + `import './authoring'; import './registration-scope';` + ), + ]; + expect( + synthetic.filter(violatesRouteAuthoringBoundary).map(format) + ).toEqual([ + 'src/router/path-policy.ts -> src/router/store.ts', + 'src/router/path-policy.ts -> src/router/authoring.ts', + 'src/router/registration-scope.ts -> src/router/route-registration.ts', + 'src/router/route-registration.ts -> src/router/authoring.ts', + ]); + }); + it('should keep query reader and cache lifetime independent of async publication', () => { expect(edges.filter(violatesQueryLifetimeBoundary).map(format)).toEqual([]); expect( diff --git a/tests/unit/router/authoring-registration-policy.test.ts b/tests/unit/router/authoring-registration-policy.test.ts new file mode 100644 index 00000000..d8fb1a2a --- /dev/null +++ b/tests/unit/router/authoring-registration-policy.test.ts @@ -0,0 +1,79 @@ +import { afterEach, describe, expect, it } from 'vite-plus/test'; +import { + createRouteRegistry, + fallback, + index, + page, +} from '../../../src/router/route'; +import type { InternalRouteRecord } from '../../../src/router/internal-types'; +import { resolveRouteFromRoutes } from '../../../src/router/route-matching'; +import { + clearRouteState, + getRouteRecords, + lockRouteRegistration, +} from '../../../src/router/store'; + +afterEach(() => clearRouteState()); + +describe('route record registration policy', () => { + it.each([ + ['index', () => index(() => null)], + ['fallback', () => fallback(() => null)], + ] as const)( + 'should reject %s registration after the active table is locked', + (_name, register) => { + lockRouteRegistration(); + expect(register).toThrow(/locked after app startup/i); + expect(getRouteRecords()).toEqual([]); + } + ); + + it.each([ + ['index', () => index({} as never)], + ['fallback', () => fallback({} as never)], + ] as const)( + 'should reject a non-function %s component without registering a record', + (_name, register) => { + expect(register).toThrow(/requires a component function/i); + expect(getRouteRecords()).toEqual([]); + } + ); + + it('should allow one valid index after a failed index registration in the same page scope', () => { + const registry = createRouteRegistry(() => { + page( + '/docs', + () => null, + () => { + expect(() => index({} as never)).toThrow( + /requires a component function/i + ); + index(() => 'overview'); + } + ); + }); + expect(registry.manifest.records.map((record) => record.path)).toEqual([ + '/docs', + ]); + }); + + it('should join a root page fallback with one slash and retain its page ownership', () => { + const Page = () => null; + const Missing = () => 'missing'; + const registry = createRouteRegistry(() => { + page('/', Page, () => { + index(() => 'home'); + fallback(Missing); + }); + }); + const record = registry.manifest.records.find( + (record) => record.isFallback + )! as InternalRouteRecord; + expect(record.path).toBe('/*'); + expect(record.fallbackPrefix).toBe('/'); + expect(record.pageChain).toEqual([{ component: Page }]); + expect(resolveRouteFromRoutes('/unknown', registry.routes)?.handler).toBe( + record.handler + ); + }); +}); diff --git a/tests/unit/router/authoring-scope-ownership.test.ts b/tests/unit/router/authoring-scope-ownership.test.ts new file mode 100644 index 00000000..eb740a12 --- /dev/null +++ b/tests/unit/router/authoring-scope-ownership.test.ts @@ -0,0 +1,193 @@ +import { afterEach, describe, expect, it } from 'vite-plus/test'; +import type { AuthContext, AuthRequirement } from '@askrjs/auth'; +import type { RouteContext } from '../../../src/common/router'; +import { + createRouteRegistry, + group, + page, + route, +} from '../../../src/router/route'; +import { resolveRouteMeta } from '../../../src/router/metadata'; +import { resolveRouteRequest } from '../../../src/router/resolution'; +import { clearRouteState } from '../../../src/router/store'; + +const auth: AuthContext = { + authenticated: true, + principal: { id: 'reader', roles: ['member'] }, + session: null, + tenant: null, +}; +const context: RouteContext = { + mode: 'ssr', + params: { id: '42' }, + pathname: '/docs/42/details', + search: '', + hash: '', + href: '/docs/42/details', + auth, + signal: new AbortController().signal, +}; + +afterEach(() => clearRouteState()); + +describe('route authoring scope ownership', () => { + it('should capture nested group and page inheritance in declaration order', async () => { + const calls: string[] = []; + const allow = + (name: string): AuthRequirement => + () => { + calls.push(name); + return { allowed: true }; + }; + const Layout = () => null; + const Page = () => null; + const InnerLayout = () => null; + const registry = createRouteRegistry(() => { + group( + { + layout: Layout, + auth: allow('outer'), + meta: { title: 'Outer', html: { lang: 'en' } }, + }, + () => { + page( + '/docs/{id}', + Page, + { + auth: allow('page'), + meta: { description: 'Page', html: { dir: 'ltr' } }, + }, + () => { + group( + { + layout: InnerLayout, + auth: allow('inner'), + meta: { title: 'Inner' }, + }, + () => { + route('details', () => null, { + auth: allow('leaf'), + meta: { title: 'Leaf' }, + }); + } + ); + } + ); + } + ); + }); + const record = registry.manifest.records[0]; + expect(record.path).toBe('/docs/{id}/details'); + expect(record.layoutChain).toEqual([ + { component: Layout }, + { component: InnerLayout }, + ]); + expect(record.pageChain).toEqual([{ component: Page }]); + expect( + ( + await resolveRouteRequest(context.pathname, { + registry, + authContext: auth, + }) + )?.kind + ).toBe('render'); + expect(calls).toEqual(['outer', 'page', 'inner', 'leaf']); + await expect(resolveRouteMeta(record, context)).resolves.toEqual({ + title: 'Leaf', + description: 'Page', + html: { lang: 'en', dir: 'ltr' }, + }); + }); + + it('should stop inherited auth at a page denial before inner auth and loaders', async () => { + const calls: string[] = []; + const registry = createRouteRegistry(() => { + group( + { + auth: () => { + calls.push('outer'); + return { allowed: true }; + }, + }, + () => { + page( + '/docs', + () => null, + { + auth: () => { + calls.push('page'); + return { allowed: false, reason: 'forbidden' }; + }, + }, + () => { + group( + { + auth: () => { + calls.push('inner'); + return { allowed: true }; + }, + }, + () => { + route('details', () => null, { + auth: () => { + calls.push('leaf'); + return { allowed: true }; + }, + loader: () => { + calls.push('loader'); + return null; + }, + }); + } + ); + } + ); + } + ); + }); + await expect( + Promise.resolve( + resolveRouteRequest('/docs/details', { registry, authContext: auth }) + ) + ).resolves.toEqual({ kind: 'deny', status: 403 }); + expect(calls).toEqual(['outer', 'page']); + }); + + it('should unwind thrown definition scopes without changing already registered records', () => { + const Layout = () => null; + const Page = () => null; + const registry = createRouteRegistry(() => { + expect(() => + group( + { + layout: Layout, + auth: () => ({ allowed: true }), + meta: { title: 'Scoped' }, + }, + () => { + page('/docs', Page, () => { + group({}, () => { + route('kept', () => null); + throw new Error('definition failed'); + }); + }); + } + ) + ).toThrow('definition failed'); + route('/after', () => null); + }); + const scoped = registry.manifest.records.find( + (record) => record.path === '/docs/kept' + )!; + const after = registry.manifest.records.find( + (record) => record.path === '/after' + )!; + expect(scoped.layoutChain).toEqual([{ component: Layout }]); + expect(scoped.pageChain).toEqual([{ component: Page }]); + expect(scoped.metaChain).toHaveLength(1); + expect(after.layoutChain).toEqual([]); + expect(after.pageChain).toEqual([]); + expect(after.metaChain).toBeUndefined(); + expect(after.options?.auth).toBeUndefined(); + }); +});