feat: native lazy entry-points + journey-graph auto-preload#30
Merged
feat: native lazy entry-points + journey-graph auto-preload#30
Conversation
Two related DX gaps in the journeys story, addressed together:
1. `defineEntry` now accepts `lazy: () => import("./Step")` directly. The
validator's "must be a function" check rejected `React.lazy()` (an exotic
object), forcing every consumer that wanted code-splitting to hand-write
a per-entry `LazyXxxStep.tsx` wrapper. The discriminated union
`EagerModuleEntryPoint | LazyModuleEntryPoint` keeps the two forms
mutually exclusive at the type level (`?: never` idiom). Hosts call a
shared `resolveEntryComponent(entry)` helper in `@modular-react/react`
that wraps lazy entries with `React.lazy` + memoizes per-entry-object
identity through a `WeakMap`, exposing an idempotent `preload()` for
speculative prefetching. `<JourneyOutlet>` and `<ModuleTab>` wrap the
resolved component in a `<Suspense>` boundary using the entry's
optional `fallback`.
2. `<JourneyOutlet>` gains a `preload` prop (default `"precise"`) that
speculatively warms reachable entries' chunks during idle time after
each step mount, so navigating Next finds the chunk hot. Two modes:
- `"precise"` reads declared `targets` from `defineTransition({ targets,
handle })`-annotated handlers on the current step's transitions and
preloads exactly those entries. Bare-function handlers contribute
nothing — the runtime can't safely run handlers speculatively, so we
trade one declarative line per transition for precision.
- `"aggressive"` falls back to "every entry referenced anywhere in
`transitions`" — useful for unmigrated journeys.
`defineTransition` attaches `targets` as a non-enumerable property and
forwards the call signature unchanged, so the runtime invocation path
at `runtime.ts:1338-1359` is untouched. `requestIdleCallback` is feature-
detected with a `setTimeout(_, 0)` fallback; SSR is a no-op.
All existing eager-component consumers continue to work unchanged —
`EagerModuleEntryPoint` is structurally identical to today's shape.
- collectRef splits on `lastIndexOf("/")` so scoped module ids
(`@scope/foo`) round-trip correctly through `${module}/${entry}` refs.
- Auto-preload effect drops `instance` from its dep list — `instance`
reference shifts on every snapshot bump (timestamps, child-id), and
re-running preload on those is wasted work. Re-keys on
`(isActive, stepModuleId, stepEntryName, journeyId)` instead.
- `fallback` moved from `ModuleEntryPointBase` onto `LazyModuleEntryPoint`
only — eager entries don't suspend, declaring it there was a DX trap.
- `cachedImport` traps a sync-throwing importer with try/catch so the
rejection is cached like any async failure (without it, a sync throw
skips the assignment and the next call re-invokes the broken importer
forever).
- Drop redundant `entry as { fallback?: ReactNode }` casts at both render
sites — `fallback` is now part of the union type.
Tests added: step-change re-runs preload set, scoped module-id splits
correctly, sync-throwing importer is cached as rejection, defineTransition
accepts handlers returning `complete` / `abort`.
…mples
- packages/journeys/README.md
- Entry-points table now documents both eager and lazy descriptor shapes.
- New "Pattern - lazy entry-points (code-splitting per step)" section under
Authoring patterns, covering the lazy importer signature, fallback,
WeakMap memoization, manual preloadEntry, error propagation, and SSR.
- New "Pattern - declared targets with defineTransition" section under
Journey definition patterns, covering both the curried (recommended)
and bare call shapes plus the type-level narrowing bonus.
- JourneyOutletProps interface gains the new `preload` prop with full
JSDoc; "What it does" enumeration now mentions Suspense + preload.
- API reference updated for the new exports + the discriminated
EagerModuleEntryPoint / LazyModuleEntryPoint / LazyEntryComponent types.
- packages/react/README.md
- "What's included" lists resolveEntryComponent + preloadEntry.
- New "Manual prefetch with preloadEntry" section showing the
onMouseEnter hover-prefetch pattern.
- defineTransition gains a curried form `defineTransition<TModules, TState>()`
that returns a typed binder. The bare `defineTransition({ targets, handle })`
still works for one-off use; the curried form is recommended for journeys
because it brings TModules into the handler's contextual return type — so
`next.module: "plan"` checks against `keyof TModules` instead of widening
to `string` (which broke assignment to the strict `transitions` slot).
- examples/{react-router,tanstack-router}/customer-onboarding-journey
- billing/CollectPayment is now lazy-loaded via
`lazy: () => import("./CollectPayment.js").then(m => ({ default: m.CollectPayment }))`.
Eliminates the bundle cost of payment collection for journeys that
branch into startTrial instead.
- Flagship transitions (profileComplete, readyToBuy, choseStandard,
choseWithTrial) are wrapped with the curried `tx = defineTransition<…>()`
binder so the outlet preloads the next-step chunks during idle time.
Other transitions stay as bare functions to demonstrate the
one-handler-at-a-time migration story.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughIntroduces lazy per-entry module entry-points ( ChangesLazy entries, declared transition targets, and auto-preload
Sequence Diagram(s)sequenceDiagram
participant JO as JourneyOutlet
participant HAR as Harvester / Target Detector
participant PL as Preloader
participant RE as resolveEntryComponent / preloadEntry
participant MT as ModuleTab (Renderer)
participant S as Suspense Renderer
JO->>HAR: Inspect current step handlers
HAR->>HAR: Extract declared targets from defineTransition({ targets })
HAR->>PL: Provide list of target entries to preload
PL->>RE: preloadEntry(targetEntry)
RE->>RE: Check WeakMap cache
alt entry cached
RE-->>PL: Return cached promise
else lazy & new
RE->>RE: Create importer promise
RE-->>PL: Return in-flight promise
else eager
RE-->>PL: Resolve immediately (no import)
end
note over JO,MT: User advances step
JO->>MT: Render step
MT->>RE: resolveEntryComponent(activeEntry)
RE->>MT: { Component, preload }
MT->>S: Render Suspense(Component, fallback)
alt lazy & chunk pending
S->>RE: await import
S-->>MT: show fallback until resolved
else chunk available or eager
S-->>MT: render Component immediately
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…rename `tx` to `transition`
Type-level regression tests, run through vitest's typecheck pass:
- packages/core/src/entry-exit.test-d.ts (NEW)
- defineEntry({ component }) returns EagerModuleEntryPoint, not the union
- defineEntry({ lazy }) returns LazyModuleEntryPoint, not the union
- declaring BOTH component+lazy fails both overloads
- declaring NEITHER fails both overloads
- `fallback` is allowed only on lazy entries (eager has `fallback: never`)
- LazyEntryComponent normalizes default-export and direct-export shapes
- ModuleEntryPoint<TInput> is exactly Eager | Lazy
- packages/react/src/resolve-entry.test-d.ts (NEW)
- resolveEntryComponent returns ResolvedEntry for both eager and lazy
- Component is renderable, preload is `() => Promise<unknown>`
- preloadEntry is a thin alias of resolveEntryComponent(...).preload()
- packages/journeys/src/define-transition.test-d.ts (NEW)
- StepRef<TModules> is `${moduleId}/${entryName}` literal union
- curried form: targets infer as literal tuple WITHOUT `as const`
- typo in targets surfaces "Did you mean" hint at compile time
- unknown module id / bare module id (no /entry) is a compile error
- handler `next.module` / `next.entry` / `next.input` all narrow against
the bound module map's entry contract
- empty targets are accepted (handlers may return complete/abort only)
- bare form: targets typed `readonly string[]` (no constraint, but the
literal tuple is still preserved by the `const TTargets` modifier)
Vitest configs in @modular-react/core and @modular-react/react now enable
`typecheck.include: ["src/**/*.test-d.ts"]` so each package owns the type
tests for the APIs it exports. Journeys was already configured this way.
Naming + type-safety polish:
- The example binder is now `const transition = defineTransition<…>()`,
not `tx`. Naming mirrors `selectModule`'s pattern (a descriptive verb,
not a magical abbreviation — `tx` reads as "transaction" in most
codebases). README + JSDoc + both example journeys updated.
- Removed redundant `as const` from `targets:` arrays in the example
journeys. The `const TTargets` modifier on the helper's generic
preserves the literal tuple type, so callers never need it. Verified
by adding the curried-form / bare-form invariant to the test-d suite.
…red targets
Two issues fixed by this change, each with its own root cause:
1. AST harvester missed wrapped handlers
The `analyzeHandler` walker only recognized `ArrowFunctionExpression` /
`FunctionExpression`. Once the example journey wrapped its flagship
handlers with `defineTransition({ targets, handle: () => (...) })`, the
handler became a `CallExpression` and the harvester returned no
destinations — the catalog e2e then failed asserting `plan` was
reachable from `profile.profileComplete`.
Fix: when the harvester sees a `CallExpression` whose first argument
is an `ObjectExpression`, look for `targets` (authoritative declared
destinations) and `handle` (recurse for `aborts` / `completes`). The
wrapper is otherwise transparent. Bare functions still work unchanged.
2. Declared targets are now first-class catalog data
When `defineTransition({ targets: [...] })` is present, the harvester
uses the targets array as the AUTHORITATIVE destination set — including
branches a pure AST walk can't resolve (e.g. `next: cond ? A : B`).
The new `targetsDeclared: boolean` flag rides through `ExitOutcome`
(config) → `ModuleExitUsage` (schema) → SPA UI, where `<DeclaredBadge>`
renders an emerald "declared" chip on `ModuleDetailView`. Authors get
visual confirmation that the destination listing is complete (vs the
AST best-effort, which is silently incomplete on dynamic returns).
E2E asserts the badge appears on `profile.profileComplete`.
Workspace pin: `@playwright/test` overridden to 1.56.1 in the root
package.json's `pnpm.overrides`. The previous `^1.58.2` resolved to 1.59.1
and required Chromium 1217, which was unavailable in the local sandbox
(Playwright 1.56.1's Chromium 1194 is what's pre-cached). CI installs
browsers fresh either way, so the version downshift is local-env-friendly
without changing CI behavior.
Tests:
- packages/catalog/src/harvester/ast-destinations.test.ts (+4 cases)
- unwraps defineTransition({ targets, handle })
- unwraps a curried-binder call (`const transition = ...; transition({...})`)
- prefers declared `targets` over AST inference (catches dynamic-return branches)
- falls back to AST inference when targets is absent
- examples/catalog/tests/catalog.spec.ts: asserts the "declared" badge.
The previous commit pinned @playwright/test to 1.56.1 via root pnpm.overrides as a local-env workaround for missing Chromium 1217 binaries. CI installs browsers fresh via `playwright install --with-deps` on whatever version resolves, so the override is unnecessary there and just held back the rest of the workspace. Lockfile reverts to playwright 1.59.1 (matching the spec ^1.58.2 across the seven e2e packages). The harvester fix from the same commit stays — that was a real bug exposed by the example journey using `defineTransition`.
…t, not slash-string)
Previously `targets:` accepted slash-strings (`"moduleId/entryName"`)
while handler returns used `next: { module, entry, input }`. The two
formats described the same idea — a (module, entry) reference — and
forcing authors to flip between them at adjacent lines was a real DX
inconsistency.
This change makes `targets:` use the same `{ module, entry }` shape
(StepSpec minus the runtime-computed `input`):
```ts
profileComplete: transition({
targets: [{ module: "plan", entry: "choose" }],
handle: ({ output, state }) => ({
next: { module: "plan", entry: "choose", input: ... },
}),
}),
```
Type-safety is preserved (and arguably improved):
- `StepRef<TModules>` is now a discriminated union of literal
`{ module, entry }` pairs, mirroring `StepSpec` minus `input`.
- Typo on `entry`, unknown `module`, missing `entry` field, and
cross-pair (entry from a different module) are all compile errors —
added @ts-expect-error coverage for each in the test-d suite.
- The bare form constraint widens to `{ module: string; entry: string }[]`
but the runtime `isAnnotatedTransition` guard structurally validates
every target object before the preloader trusts it.
Plumbing:
- `collectPreloadTargets` (outlet.tsx) — drops the `lastIndexOf("/")`
parser; reads `target.module` / `target.entry` directly. Dedupe key is
now a space-separated composite, so module ids that legitimately
contain `/` (npm-style scopes) are no longer a special case.
- `readDeclaredTargets` (ast-destinations.ts) — parses object literals
instead of slash-strings; each target requires both `module` and
`entry` literal-string properties.
- `attach` deep-freezes each target object as well as the array, so a
caller can't mutate `targets[0].module` after the fact.
Tests + docs updated: define-transition.test{,-d}.ts (10 type-level
checks now exercise object-shape narrowing), outlet-preload.test.tsx,
ast-destinations.test.ts, both example journeys, README, JSDoc.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/journeys/README.md`:
- Around line 2063-2065: Update the README API table entry for defineExit to
match the implementation: replace the documented optional-arg signature with the
zero-argument generic form used in code (i.e., defineExit has signature `<T =
void>() => ExitPointSchema<T>`), and keep the note that it is an identity
helper/zero runtime cost; ensure the table cell text for `defineExit` exactly
reflects the no-argument generic form so users aren't misled by an example that
suggests a parameter is accepted.
In `@packages/journeys/src/outlet-preload.test.tsx`:
- Around line 14-22: The test deletes globalThis.requestIdleCallback in
beforeEach which leaks into other tests; capture the original value (e.g. const
originalRIC = (globalThis as { requestIdleCallback?: unknown
}).requestIdleCallback) before you delete it in the beforeEach, then restore it
in afterEach (alongside cleanup()) by reassigning globalThis.requestIdleCallback
= originalRIC (or deleting it if it was undefined) so the global environment is
returned to its prior state; update the beforeEach/afterEach blocks in
outlet-preload.test.tsx accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b14ddc1b-49c3-4d1b-a751-838cfe9bfeeb
📒 Files selected for processing (33)
examples/catalog/tests/catalog.spec.tsexamples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/customer-onboarding.tsexamples/react-router/customer-onboarding-journey/modules/billing/src/index.tsexamples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.tsexamples/tanstack-router/customer-onboarding-journey/modules/billing/src/index.tspackages/catalog/spa-src/src/types.tspackages/catalog/spa-src/src/views/ModuleDetailView.tsxpackages/catalog/src/config/types.tspackages/catalog/src/harvester/ast-destinations.test.tspackages/catalog/src/harvester/ast-destinations.tspackages/catalog/src/schema/build-model.tspackages/catalog/src/schema/types.tspackages/core/src/entry-exit.test-d.tspackages/core/src/entry-exit.test.tspackages/core/src/entry-exit.tspackages/core/src/index.tspackages/core/src/types.tspackages/core/vitest.config.tspackages/journeys/README.mdpackages/journeys/src/define-transition.test-d.tspackages/journeys/src/define-transition.test.tspackages/journeys/src/define-transition.tspackages/journeys/src/index.tspackages/journeys/src/module-tab.test.tsxpackages/journeys/src/module-tab.tsxpackages/journeys/src/outlet-preload.test.tsxpackages/journeys/src/outlet.tsxpackages/react/README.mdpackages/react/src/index.tspackages/react/src/resolve-entry.test-d.tspackages/react/src/resolve-entry.test.tsxpackages/react/src/resolve-entry.tspackages/react/vitest.config.ts
…ack in tests - packages/journeys/README.md defineExit's API row claimed `<T = void>(s?: ExitPointSchema<T>)` but the implementation has been zero-arg since inception (entry-exit.ts:50). Aligned the table to the actual signature. - packages/journeys/src/outlet-preload.test.tsx beforeEach deleted globalThis.requestIdleCallback without restoring it. On shared test workers a later suite that relies on a shimmed RIC could see the deletion. Capture the original at import time, restore in afterEach.
…BREAKING]
`targets` now accepts a mixed array of `{ module, entry }` step refs and
`"complete"` / `"abort"` / `"invoke"` string sentinels. The handler return
type is constrained to the declared arms — returning `abort` when only
`next` was declared is a compile error.
```ts
const transition = defineTransition<OnboardingModules, OnboardingState>();
profileComplete: transition({
targets: [
{ module: "plan", entry: "choose" },
{ module: "billing", entry: "collect" },
],
handle: ({ output, state }) =>
output.hint === "cheap"
? { next: { module: "plan", entry: "choose", input: ... } }
: { next: { module: "billing", entry: "collect", input: ... } },
}),
cancelled: transition({
targets: ["abort"],
handle: () => ({ abort: { reason: "user-cancelled" } }),
}),
review: transition({
targets: [{ module: "plan", entry: "choose" }, "abort"],
handle: ({ output }) =>
output.ok
? { next: { module: "plan", entry: "choose", input: ... } }
: { abort: { reason: "rejected" } },
}),
```
BREAKING:
- `targets` is now mandatory on every `defineTransition` call. The wrapper's
whole point is to enumerate possible outcomes; an empty/missing array
would silently sit out of precise-mode preload while looking annotated.
Bare-function handlers (`exitName: () => ({...})`) are unchanged — use
those when you don't want to declare outcomes.
- @modular-react/journeys bumped to 1.0.0; consuming packages updated to
`^1.0.0`. The `^0.1.0` ranges are no longer compatible with the new
surface, and the change was deliberately staged here rather than left
to silent compat surprise.
Catalog harvester:
- `aborts` / `completes` flags now come from declared sentinels rather
than walking the handler body. Authoritative — no AST guesswork on
dynamic returns.
- `"invoke"` sentinel is parsed but doesn't yet flow to a schema flag;
added so handlers that may invoke aren't rejected today.
Type-level coverage in *.test-d.ts asserts: typo on a sentinel is a
compile error, returning an arm not declared in `targets` is a compile
error (both next-only and terminal-only directions), step-ref + sentinel
mixing infers the right `targets` literal tuple, and `StepRef<TModules>`
expands to the union of valid refs plus the three sentinels.
Typed-`invoke` variant (which would constrain to one of the journey's
declared `invokes` handles) is intentionally NOT shipped here — adds a
4th generic to the curried binder, expects narrowly-typed `invokes`
arrays from authors, and offers marginal real-world value over the
simple sentinel. Logged as a future enhancement.
Examples (both customer-onboarding-journey shells) demonstrate the
sentinel: the `cancelled` handler is now wrapped with
`transition({ targets: ["abort"], handle })` so the catalog UI surfaces
the "declared" badge on it end-to-end.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/catalog/src/harvester/ast-destinations.test.ts`:
- Line 197: The test duplicates the temp filename "wrapped.ts" used by the
"works through a defineJourney(...)({...}) wrapper" test, risking collisions;
change the writeTmp call in this test to a unique filename (e.g., "wrapped-2.ts"
or a name derived from the test title) and update any assertions/imports that
reference that path so the test no longer shares the same temp file;
alternatively, replace the literal filename with a helper that generates unique
temp names to avoid future collisions (modify the writeTmp call and related
references).
In `@packages/journeys/src/define-transition.ts`:
- Around line 137-154: The attach function currently mutates spec.handle by
defining a non-configurable "targets" property which causes a cryptic TypeError
if the same function is reused; update attach to first check
Object.getOwnPropertyDescriptor(handler, "targets") and if the property already
exists throw a clear, actionable Error (including the handler name and
instruction to use a fresh function or clone) instead of attempting
Object.defineProperty again, otherwise proceed to define the non-enumerable
non-configurable "targets" as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d8ffe92-2a47-495d-bb5c-1de24f25b081
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
examples/react-router/customer-onboarding-journey/journeys/customer-onboarding/package.jsonexamples/react-router/customer-onboarding-journey/journeys/customer-onboarding/src/customer-onboarding.tsexamples/react-router/customer-onboarding-journey/shell/package.jsonexamples/react-router/journey-invoke/journeys/checkout/package.jsonexamples/react-router/journey-invoke/journeys/verify-identity/package.jsonexamples/react-router/journey-invoke/shell/package.jsonexamples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/package.jsonexamples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.tsexamples/tanstack-router/customer-onboarding-journey/shell/package.jsonexamples/tanstack-router/journey-invoke/journeys/checkout/package.jsonexamples/tanstack-router/journey-invoke/journeys/verify-identity/package.jsonexamples/tanstack-router/journey-invoke/shell/package.jsonpackages/catalog/src/harvester/ast-destinations.test.tspackages/catalog/src/harvester/ast-destinations.tspackages/journeys/README.mdpackages/journeys/package.jsonpackages/journeys/src/define-transition.test-d.tspackages/journeys/src/define-transition.test.tspackages/journeys/src/define-transition.tspackages/journeys/src/index.tspackages/journeys/src/outlet-preload.test.tsxpackages/journeys/src/outlet.tsxpackages/react-router-testing/package.jsonpackages/tanstack-router-testing/package.json
✅ Files skipped from review due to trivial changes (12)
- examples/tanstack-router/customer-onboarding-journey/shell/package.json
- packages/journeys/package.json
- packages/react-router-testing/package.json
- examples/tanstack-router/journey-invoke/shell/package.json
- examples/tanstack-router/journey-invoke/journeys/checkout/package.json
- examples/react-router/customer-onboarding-journey/journeys/customer-onboarding/package.json
- packages/tanstack-router-testing/package.json
- examples/react-router/journey-invoke/journeys/verify-identity/package.json
- examples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/package.json
- examples/react-router/customer-onboarding-journey/shell/package.json
- examples/react-router/journey-invoke/journeys/checkout/package.json
- examples/tanstack-router/journey-invoke/journeys/verify-identity/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/journeys/src/index.ts
- packages/journeys/src/define-transition.test-d.ts
- packages/journeys/src/outlet-preload.test.tsx
- packages/journeys/README.md
- examples/tanstack-router/customer-onboarding-journey/journeys/customer-onboarding/src/index.ts
…reuse in defineTransition
- packages/catalog/src/harvester/ast-destinations.test.ts
Two tests wrote `writeTmp("wrapped.ts", ...)` to the same path under
tmpRoot. Sequential runs hide the collision today, but parallelisation
or reordering would interfere. Renamed the defineTransition-flavour
fixture to `wrapped-define-transition.ts`.
- packages/journeys/src/define-transition.ts
Reusing the same handler reference across two `defineTransition` calls
used to throw `TypeError: Cannot redefine property: targets` (the
property is non-configurable so the second `Object.defineProperty`
call fails). Detect the prior stamp explicitly and surface an
actionable message instead. Added a test that asserts the friendly
error fires.
…is mandatory
`analyzeHandler`'s CallExpression branch used to recurse into the inner
`handle:` function when `targets` was absent, on the theory that the
wrapper might just be `defineTransition({ handle })` without targets.
That's no longer possible: `targets` is mandatory on every
`defineTransition` invocation in the runtime. A CallExpression without
`targets` is therefore some unrelated helper the harvester has no
contract with — recursing into its inner handler would falsely surface
destinations for a wrapper that may not even forward the function
verbatim.
Simplified branch: if `targets:` is present and parses, classify;
otherwise treat as opaque. Test updated to assert opacity instead of
the old fallback behavior.
Collaborator
|
Suggestion: synchronous-thenable fast path on cached reads Big +1 on this PR — the The trace, when preload completes before a step renders: The The fix: dedupe + synchronous-thenable cached path Wrap the loader with a function makeMemoizedLoader<T>(loader: () => Promise<{ default: ComponentType<T> }>) {
let cached: { default: ComponentType<T> } | undefined
let inflight: Promise<{ default: ComponentType<T> }> | undefined
return () => {
if (cached) {
const value = cached
return {
then(onFulfilled?: (m: typeof value) => unknown) {
return onFulfilled ? onFulfilled(value) : value
},
} as unknown as Promise<{ default: ComponentType<T> }>
}
inflight ??= loader().then((m) => {
cached = m
return m
})
return inflight
}
}Then const memoLoader = makeMemoizedLoader(entry.lazy)
return {
Component: React.lazy(memoLoader),
preload: memoLoader, // populates `cached` for future lazy reads
}Two things that aren't obvious:
Test sketch Wrap Suspense with a counter, render const FallbackCount = vi.fn()
function CountingSuspense({ children, fallback }) {
return <Suspense fallback={<><FallbackCount />{fallback}</>}>{children}</Suspense>
}
it('does not flash the fallback after preload completes', async () => {
const journey = makeTestJourney()
const runtime = makeRuntime({ journey })
const startStep = runtime.start(journey, ...)
await runtime.preloadEntry(/* the next step */) // fully await
render(<JourneyOutlet instanceId={startStep} />, { wrapper: CountingSuspense })
// … advance to the preloaded step …
expect(FallbackCount).not.toHaveBeenCalled()
})Lint note
// biome-ignore lint/suspicious/noThenProperty: synchronous thenable for React.lazy fast-path
then(onFulfilled?: ...) { ... }Happy to open a follow-up PR against this branch with the wrapper + the test if useful. |
diogomiguel
reviewed
May 7, 2026
Comment on lines
+406
to
+410
| // Aggressive — every (module, entry) referenced as a transition source. | ||
| for (const [moduleId, perModule] of Object.entries(transitions)) { | ||
| if (!perModule) continue; | ||
| for (const entryName of Object.keys(perModule)) collectPair(moduleId, entryName); | ||
| } |
Collaborator
There was a problem hiding this comment.
Minor — this enumerates transition keys (source steps), but the docblock says "every entry referenced anywhere in the journey's transitions map". For typical journeys those sets coincide (every reachable destination eventually becomes a source unless terminal), but a destination that's never declared as a source — for example a brand-new step reachable from start but with no outbound transitions wired yet — slips through and never preloads.
Walking definition.start plus the destinations inside each handler's targets (when annotated) would close the gap, e.g.:
// destinations from annotated transitions
for (const perModule of Object.values(transitions)) {
if (!perModule) continue
for (const perEntry of Object.values(perModule)) {
if (!perEntry) continue
for (const value of Object.values(perEntry)) {
if (!isAnnotatedTransition(value)) continue
for (const target of value.targets) {
if (typeof target === 'string') continue
collectPair(target.module, target.entry)
}
}
}
}Then union with the source-side iteration as a fallback for bare-function handlers — same set in practice, plus the destination-only edge case. Or update the docblock to say "every entry that has outbound transitions" and call the start step out separately.
Not a blocker — just an authoring-time gotcha that's easy to forget once a journey is in flight.
Owner
Author
There was a problem hiding this comment.
Good catch — addressed in d2dcb9e. The aggressive branch now walks annotated handlers' targets arrays alongside the source-side keys, so any destination declared via a next: ref is in the preload set even when its own outbound transitions are empty. Added a test (outlet-preload.test.tsx — "preloads destination-only entries reached via annotated targets even when they have no outbound transitions") that exercises the exact shape you described.
I left definition.start out of scope intentionally:
- It's a function, and running it speculatively to capture the start step is the same risk we deliberately avoided for handler bodies (state/input fabrication, possible side effects).
- Bundling a JS parser into the runtime to AST-walk
start.toString()is too heavy. - The start step is almost always the current step at first mount, where aggressive mode skips it anyway. Re-entering via the same outlet without re-mount is the only scenario where it would matter.
- The destinations-side pass already covers any step reachable from an annotated transition — authors who care about the start step can annotate the predecessor exit (or
startitself if approached from elsewhere) and get the preload for free.
Updated the docblock to call out the start-step caveat explicitly so it's not surprising to authors who later add a step reachable only via start. Happy to add an opt-in definition.startsAt: declaration if you'd prefer hard coverage of that case — say the word.
Generated by Claude Code
Owner
Author
There was a problem hiding this comment.
Update on the start exploration: tried both routes and walked back from each. Final state in 0b9578a leaves only the destinations-side fix from d2dcb9e and a clarified docblock.
startsAt field — viable but redundant in practice (see below). Pulled back.
Consuming start indirectly via instance.history[0] — the runtime has the start step the moment the journey advances, so we can read it without re-running the function. Wired it up briefly… then realised the same logic problem applies:
For the history-based pass to add anything beyond source-side enumeration, you'd need a step that's both in
instance.history[0](so the journey advanced away from it) AND not a key indefinition.transitions(so source-side misses it). Those conditions contradict — any step you advance away from must have had an outbound transition fire, which makes it a transition source. Source-side already catches it.
The remaining static gap — "step reachable only via definition.start AND with no outbound transitions of its own" — is unobservable: such a step can only ever be the current step on first mount (no exits → no advance → history stays empty), and the skip-current logic already excludes it (correctly: it's already mounted). Trying to "fix" it would just preload the chunk we're rendering.
So the practical answer is: there's nothing to close here. Updated the docblock to say so plainly rather than imply a real gap exists. The destinations-side pass from d2dcb9e stays — that one IS observable (terminal-only destination steps reachable from annotated next: arms).
Apologies for the meander — appreciate the push to think it through. If you see a real-world shape I'm missing, happy to revisit.
Generated by Claude Code
… targets `collectPreloadTargets` in `"aggressive"` mode previously enumerated only the source-side keys of `definition.transitions` — meaning a step that's a destination of some annotated `next` arm but has no outbound transitions of its own (e.g. a freshly-added receipt screen) silently slipped through preload despite the docblock claiming "every entry referenced anywhere." Now we also walk every annotated handler's `targets` and union the object-shaped refs with the source-side set. The standard `seen` dedupe keeps the work O(unique entries). `definition.start` remains out of scope: it's a function and we deliberately don't run it speculatively or bundle a JS parser. The docblock now calls this out explicitly, and authors who care can annotate any transition that lands on the start step from elsewhere — the destinations-side pass picks that up.
…story plumbing After exploring `definition.startsAt` and an `instance.history[0]`-based plumbing path to close the start-only-reachable gap, both turned out to be redundant in practice: - A step that's been navigated AWAY from must have had an outbound transition fire, which makes it a key in `definition.transitions` — source-side enumeration already covers it. - A step with no outbound transitions can't be navigated away from, so history stays empty and it remains the current step — skip-current logic excludes it (correctly: it's already mounted). The "start step reachable only via start AND with no outbound transitions" combination is therefore impossible to hit in real journeys, and any plumbing to "fix" it ships dead code. Reverted those changes; updated the docblock + JSDoc to acknowledge the residual edge as a documented non-issue (you can't observe the gap because the step is already mounted). The destinations-side fix from d2dcb9e (terminal-only destinations reachable via annotated `next:` arms) stays — that one IS observable and adds real value.
… reads
Previously, a render that landed AFTER `preload()` had fully resolved
still flashed the Suspense fallback for one microtask:
preload() // loader() → P1; P1 resolves
[later: outlet renders <Inner />]
React.lazy._init()
├── calls cachedImport() // returns cached real promise
├── calls .then(setResolved) // schedules cb in next microtask
├── checks _status → Pending // status still Uninitialized → Pending
└── throws thenable // Suspense renders fallback
[microtask boundary]
└── setResolved fires → Resolved
[next render] // real component appears
Spec-compliant fix: once the import settles, return a SYNCHRONOUS
thenable from the cached path. React.lazy's `.then(setResolved)` runs
`setResolved(value)` synchronously, flipping `_status` to `Resolved`
before `_init`'s next status check — no microtask boundary, no
suspending throw, no fallback flash. The Promise A+ spec only requires
`.then` to *schedule* its callback; React doesn't depend on the
deferral.
Two load-bearing details:
1. `Component` (via React.lazy) and `preload` share the same
`cachedImport` closure, so a `cached` slot populated by an explicit
`preload()` is visible to the subsequent lazy-render. If they got
separate closures, the cache would be invisible to lazy and the
flash would persist.
2. Separate `inflight` slot dedupes concurrent loader calls — when
`preload()` and a render fire back-to-back (hover-prefetch into
click), both share one `import()` call instead of double-fetching.
Tests added:
- "does not flash the Suspense fallback after `preload()` has settled"
— counts fallback mounts, asserts zero after preload+render.
- "dedupes concurrent loader calls so `preload()` and a render share
one import" — asserts importer called exactly once when both fire
in the same tick.
The synchronous thenable trips `unicorn/no-thenable`; suppressed inline
with a comment that points at the block-comment rationale, since the
construct is deliberate and load-bearing.
Credit: @diogomiguel for the diagnosis + the implementation sketch.
Owner
Author
|
@diogomiguel — landed in 2edb1f6, exactly the shape you sketched. Two load-bearing details from your message that I made sure to honor:
Skipped the rejection-path mirror for now — when an import fails the user lands in the error boundary anyway, so whether the fallback flashes for a microtask first is secondary. Easy to add later if real apps hit it. Tests added (
For the lint note, the repo uses oxlint rather than biome; the inline directive is Thanks for the careful diagnosis with the trace + the spec-compliance reasoning — turned a "we don't preload visibly enough" gap into a small focused change with assertable tests. Generated by Claude Code |
This was referenced May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related DX gaps in the journeys story, addressed together. All existing eager-component consumers and bare-function transition handlers continue to work unchanged.
defineEntrynow acceptslazy: () => import("./Step")directly, eliminating the per-entryLazyXxxStep.tsxwrapper consumers had to write.<JourneyOutlet>speculatively warms reachable entries' chunks during idle time after each step mount, so navigating Next finds the chunk hot.Bumps
@modular-react/journeysto 1.0.0 —defineTransition.targetsis mandatory and the handler return is narrowed against it (see "Breaking changes" below).Native lazy entry-points
Why this didn't exist before: the validator's "must be a function" check rejected
React.lazy()(an exotic object), so authors hand-wroteLazyXxxStep.tsxwrappers.Key design choices:
EagerModuleEntryPoint | LazyModuleEntryPointis a discriminated union, mutually exclusive at the type level via?: neveron each branch. A descriptor that declares both — or neither — fails bothdefineEntryoverloads.fallbacklives only on the lazy branch, typedneveron the eager one. Eager entries don't suspend; allowing the field there would silently no-op and confuse authors.resolveEntryComponent(entry)helper memoizes per-entry-object identity through aWeakMap, returns{ Component, preload }, and traps sync-throwing importers via try/catch so the rejection is cached like any async failure (without it, a sync throw would re-invoke the broken importer on every call). Both<JourneyOutlet>and<ModuleTab>go through it, so the lazy wrapper is shared across renders, hot reloads, and StrictMode double-mount.Auto-preload from the journey graph
<JourneyOutlet>gainspreload?: boolean | "precise" | "aggressive"(default"precise"):"precise"— read declaredtargetsfromdefineTransition-annotated handlers on the current step's transitions and preload exactly those entries (sentinel targets are skipped — they have no chunk). Bare-function handlers contribute nothing."aggressive"— preload every entry referenced anywhere intransitions. Useful for unmigrated journeys.false— opt out.Why opt-in declarations rather than inferring targets from handler bodies: handler bodies are dynamic (
next: cond ? A : B) and may have side effects, so the runtime can't safely run them speculatively. One declarative line per transition is the trade-off for precision. The effect runs after the step settles in-DOM and cancels on step change so a fast advance doesn't race with the previous set.requestIdleCallbackis feature-detected with asetTimeout(_, 0)fallback; SSR is a no-op.defineTransitiontargets:accepts a mixed array of{ module, entry }step refs (same shape asnext:minus the runtime-computedinput) and"complete"/"abort"/"invoke"string sentinels for the terminal arms.targets— declaring onlynextarms forbidsabort/completereturns at compile time.TModulesinto the handler's contextual return type, sonext.module/next.entryand the choice of arms are checked against the bound types instead of widening tostring/ accepting any arm.defineTransition({ targets, handle })works for one-off use without binding generics.targetsas a non-enumerable, deeply-frozen property and forwards the call signature unchanged, so runtime dispatch is untouched. Reusing the same handler reference across twodefineTransitioncalls throws an actionable error rather than the crypticCannot redefine propertyengine error. The"invoke"sentinel doesn't constrain to a specific child journey — the journey'sinvokes:field remains the closed-set declaration the runtime cycle-guards check against.Catalog integration
The AST harvester recognizes
defineTransition-wrapped handlers and readstargetsdirectly — both the next-step destinations (object refs) and the terminalaborts/completesflags (string sentinels). Withtargetsmandatory, the harvester no longer walks handler bodies for wrapped exits at all; the declaration is the contract. Bare-function handlers are still AST-walked as before. ThetargetsDeclared: booleanflag rides through to the schema → SPA so the catalog UI can mark wrapped exits with a "declared" badge — useful when a single page renders both wrapped and bare handlers.Breaking changes
defineTransition({ targets, handle })—targetsis now mandatory. The wrapper's whole point is to enumerate possible outcomes; an empty/missing array would silently sit out of precise-mode preload while looking annotated. If you don't want to declare outcomes, use a bare function — the runtime invocation path is identical.abortfrom a branch when onlynextwas declared). Add"abort"totargetsto allow it.@modular-react/journeysbumped to 1.0.0; consuming packages updated to^1.0.0.A typed
"invoke"variant (constraining to one of the journey's declaredinvokeshandles) is intentionally NOT shipped here — adds a 4th generic to the curried binder, expects narrowly-typedinvokesarrays from authors, and offers marginal value over the simple sentinel. Logged as a future enhancement.Test plan
pnpm -r typecheckclean across all packages.pnpm -r test— 928+ workspace tests pass.*.test-d.tsfiles (vitest typecheck pass) lock indefineEntryoverload selection,targetstypo / unknown-module / missing-field / cross-pair compile errors, sentinel narrowing (returning an undeclared arm is a compile error), and step-ref + sentinel mixing — every failure mode@ts-expect-error-asserted.pnpm exec oxfmt --check .andpnpm exec oxlint .clean.pnpm --filter @example/catalog test:e2e— 12/12 pass; the new "declared" badge is asserted onprofile.profileCompleteof the example journey, and the harvester verifies sentinel-derivedaborts: trueon the example'scancelledhandler.https://claude.ai/code/session_01A1XVeV1bxBYZ43jM1J5Byx