-
Notifications
You must be signed in to change notification settings - Fork 114
fix(plugin-history-sync): unify useActivityParams runtime type across navigation paths #705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6654f86
737cc13
3c1591a
7462618
6ab0f0a
45eeed6
d72fe49
085210c
33df89c
3a6476f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| --- | ||
| "@stackflow/plugin-history-sync": minor | ||
| --- | ||
|
|
||
| Coerce activity/step params to `string | undefined` at the plugin boundary. | ||
|
|
||
| Before this change, `push("X", { visible: true })` would store the boolean `true` in the core store while URL-arrival parsed the same URL as `{ visible: "true" }`, so `useActivityParams<K>()` returned different runtime types depending on how the user reached the activity. This PR coerces non-string values to strings inside `plugin-history-sync`'s `onBeforePush` / `onBeforeReplace` / `onBeforeStepPush` / `onBeforeStepReplace` hooks (after `encode` consumes the typed params to build the URL), and on the `decode`-path in `overrideInitialEvents`, so the core store always contains `{ [key: string]: string | undefined }`. `encode` still receives the typed params `U` from `template.fill`. Post-effect hooks (`onPushed`, `onReplaced`, `onStepPushed`, `onStepReplaced`, `onInit`) now use the new `fillWithoutEncode` to avoid re-running `encode` on already-coerced store values. | ||
|
|
||
| This is a behavioral change for consumers that relied on internal push preserving non-string values in the store (a pre-existing divergence from URL-arrival behavior). See the docs update for the migration note. | ||
|
|
||
| Migration notes: | ||
|
|
||
| - If you authored a `decode` hook that returns typed values (e.g. `decode: (p) => ({ count: Number(p.count) })`), those return values are now coerced back to strings in the store to match the declared `ActivityBaseParams` contract. Move runtime type coercion to the usage site (`Number(useActivityParams().count)`). | ||
| - If your app registers a plugin AFTER `historySyncPlugin` in the plugins array and that plugin re-injects typed values via `overrideActionParams`, those values will NOT be coerced by this plugin. Register `historySyncPlugin` last among plugins that mutate `activityParams` to preserve the string-only invariant. | ||
| - Cross-deploy hydration: when a user reloads on a deploy that includes this fix after a previous deploy serialized typed values into `history.state`, the params are coerced to strings at hydration time inside the `parseState` early-return. No consumer change required — the post-fix runtime contract (`useActivityParams()` returns `string | undefined`) holds across version boundaries. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| --- | ||
| "@stackflow/core": minor | ||
| "@stackflow/plugin-history-sync": patch | ||
| --- | ||
|
|
||
| Add optional `stepContext.path?: string` to `StepPushedEvent` and `StepReplacedEvent` (purely additive, no breaking change). `@stackflow/plugin-history-sync` uses this to preserve `encode`-output URLs through the store across every step navigation path — including `popstate` forward across step boundaries — instead of relying on plugin-internal state. | ||
|
|
||
| This addresses three regressions surfaced in PR review: | ||
|
|
||
| 1. **`encode` output not in `history.location`** — post-effect hooks (`onPushed` / `onReplaced` / `onStepPushed` / `onStepReplaced` / `onInit`) called `template.fillWithoutEncode(activity.params)` against the post-coercion strings, skipping `encode` and writing coerced values into the URL. Now they read the encoded URL pre-computed in pre-effect hooks (`activityContext.path` / `stepContext.path`), with `fillWithoutEncode` as a defensive fallback only. | ||
| 2. **`encode` called with coerced strings on popstate forward re-push** — the popstate `isForward` and `isStepForward` branches reconstructed push events without preserving `activityContext` / `stepContext`, causing `onBeforePush` / `onBeforeStepPush` to call `template.fill` with already-coerced strings. Now those branches pass `activityContext: targetActivity.context` / `stepContext: targetStep.context`, and the pre-effect hooks short-circuit when the path is already present (`"path" in actionParams.activityContext`). | ||
| 3. **Test gap: `path(history.location)` was never asserted under non-identity `encode`** — every existing test asserted `activity.context.path` only. Added 15 new tests asserting the URL surface under non-identity encode, including popstate-forward across activity AND step boundaries, `defaultHistory` ancestor URLs, SSR replay, and `replace`-with-active-steps. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -126,6 +126,22 @@ declare module "@stackflow/config" { | |||||
| } | ||||||
| ``` | ||||||
|
|
||||||
| ### 액티비티 파라미터의 런타임 강제 변환 (FEP-1061) | ||||||
|
|
||||||
| 액티비티에 어떻게 진입했는지에 관계없이 — `push()`, `replace()`, `stepPush()`, `stepReplace()`, 또는 URL로의 직접 진입(`decode` 유무와 무관) — `useActivityParams()`로 받는 파라미터는 런타임에 항상 `string | undefined` 예요. | ||||||
|
|
||||||
| ```tsx | ||||||
| // 이전에는 두 경로가 런타임에 서로 다른 타입을 반환했지만, 이제는 동일해요. | ||||||
| push("Article", { visible: true }) // 스토어: { visible: "true" } | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 예시 액티비티 이름을 문서 내 다른 예시와 맞춰주세요. Line 135의 Suggested patch-push("Article", { visible: true }) // 스토어: { visible: "true" }
+push("ArticleActivity", { visible: true }) // 스토어: { visible: "true" }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||
| // URL 진입: /articles/1?visible=true // 스토어: { visible: "true" } | ||||||
| ``` | ||||||
|
|
||||||
| 라우트의 `encode` 훅은 여전히 원본의 타입이 적용된 파라미터 `U`를 받아요 (예: `encode: ({ visible }) => ({ visible: visible ? "y" : "n" })`는 그대로 동작해요). 문자열화는 `encode`가 URL 생성을 위해 타입이 적용된 값을 소비한 *이후에*, `@stackflow/plugin-history-sync` 경계에서 이뤄져요. | ||||||
|
|
||||||
| <Callout emoji="⚠️"> | ||||||
| **`decode` 사용자를 위한 마이그레이션 안내**: 이전에 `decode`로 타입이 적용된 값을 주입해서 (예: `decode: (p) => ({ count: Number(p.count) })`) `useActivityParams().count`를 숫자로 사용하셨다면, 이제 해당 값은 스토어에서 문자열이에요. 사용 지점에서 타입 변환을 해주세요: `Number(params.count)`. | ||||||
| </Callout> | ||||||
|
|
||||||
| ## `useFlow()`, `useStepFlow()` | ||||||
| 이제 `flow()` 등의 함수로 `useFlow()`, `useStepFlow()` 등의 훅을 생성할 필요가 없어요. 바로 import해서 쓸 수 있어요. | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
| # Activity params runtime contract — design intent | ||
|
|
||
| This document captures the **chosen interpretation** and **boundary decision** for activity / step params runtime types in `@stackflow/plugin-history-sync`. It exists to record why the runtime contract was tightened to "always string" rather than widened to allow arbitrary types. | ||
|
|
||
| ## Chosen interpretation | ||
|
|
||
| **Always-string at the plugin boundary.** `useActivityParams<K>()` returns `Record<string, string | undefined>` regardless of how an activity is entered — `push` / `replace` / `stepPush` / `stepReplace` / URL arrival with or without `decode` / cross-deploy `historyState` hydration. | ||
|
|
||
| The originating user request was not "let me pass typed values into the store" but rather "stop the auto-typecast that makes `useActivityParams` return a different runtime type depending on entry path". Internal navigation (`push({ visible: true })`) used to leave a boolean in the store while URL-arrival parsed the same value as `"true"`. Consumers were string-converting params before every push as a workaround. | ||
|
|
||
| This implementation does **not** widen `ActivityBaseParams = { [key: string]?: string }`; it enforces that declaration at runtime so the type and the runtime stop diverging. | ||
|
|
||
| ## Boundary decision | ||
|
|
||
| **Coercion lives at the `@stackflow/plugin-history-sync` boundary, not in `@stackflow/core`.** | ||
|
|
||
| Concretely, `coerceParamsToString` runs inside the plugin's pre-effect hooks (`onBeforePush`, `onBeforeReplace`, `onBeforeStepPush`, `onBeforeStepReplace`) and inside `overrideInitialEvents` for the URL-arrival and cross-deploy hydration paths. `@stackflow/core` itself does not coerce. | ||
|
|
||
| ### Tradeoff | ||
|
|
||
| A consumer that uses `@stackflow/core` *without* `historySyncPlugin` (e.g., programmatic-only navigation with no URL sync) does NOT receive the coercion. That consumer's store will contain whatever typed values they passed to `push()`. This is a documented tradeoff: | ||
|
|
||
| - **Pro:** keeps `@stackflow/core` framework-agnostic and free of plugin-specific concerns. | ||
| - **Pro:** consumers who don't need URL sync don't pay the coercion cost. | ||
| - **Con:** the invariant is plugin-conditional, not core-universal. Consumers swapping `historySyncPlugin` for a different sync plugin must implement equivalent coercion. | ||
|
|
||
| ### Why this tradeoff was chosen | ||
|
|
||
| 1. `historySyncPlugin` is the only first-party plugin that serializes params to a string-shaped destination (URL). Without that destination, string-coercion has no architectural justification. | ||
| 2. Moving coercion into `@stackflow/core` would make the core opinionated about a serialization concern that's strictly a plugin's responsibility. | ||
| 3. The `ActivityBaseParams` type declaration (`{ [key: string]?: string }`) already pins consumer expectations at compile time; runtime coercion at the plugin boundary brings the runtime into alignment with the declared type, but the type itself remains the source of truth for non-history-sync consumers. | ||
|
|
||
| If a future requirement establishes that the invariant should be a core-store contract instead, the implementation has to move into `@stackflow/core`'s reducer and the `coerceParamsToString` utility migrates with it. | ||
|
|
||
| ## Plugin order matters (documented limitation) | ||
|
|
||
| If a plugin registered AFTER `historySyncPlugin` in the plugins array calls `overrideActionParams` with typed values, those values bypass `historySyncPlugin`'s pre-effect coercion and land in the store as-is. This is locked as a regression test so it cannot silently regress. | ||
|
|
||
| **Consumer guidance:** register `historySyncPlugin` last among plugins that mutate `activityParams`. The changeset for this work documents this. | ||
|
|
||
| ## Cross-deploy hydration | ||
|
|
||
| `overrideInitialEvents`'s `parseState` early-return deserializes activity / step state previously written to `history.state`. If an old deploy wrote typed values, the new deploy's `coerceParamsToString` calls coerce them at hydration time. Idempotent on already-coerced strings. | ||
|
|
||
| ## URL output contract — `history.location` reflects `encode` output | ||
|
|
||
| The runtime contract for `useActivityParams()` is "always string". The contract for `history.location` is **independent**: it reflects `encode` output for routes with a custom `encode`. | ||
|
|
||
| To uphold both contracts: | ||
|
|
||
| 1. Pre-effect hooks (`onBeforePush` / `onBeforeReplace` / `onBeforeStepPush` / `onBeforeStepReplace`) compute the encoded URL via `template.fill(typed_params)` BEFORE coercion. Activities store this in `activityContext.path` (already in core); steps store it in `stepContext.path`. | ||
| 2. Post-effect hooks (`onPushed` / `onReplaced` / `onStepPushed` / `onStepReplaced` / `onInit`) read `activity.context.path` and `step.context.path` directly. They never re-run `encode` on coerced strings. | ||
| 3. The popstate `isForward` and `isStepForward` branches preserve `activityContext` / `stepContext` from the stored target, so the encoded URL is recovered without re-running `encode`. | ||
| 4. If `*.context.path` is missing (e.g. a third-party plugin dispatched a `Pushed` event without going through `onBeforePush`, or a pre-update `history.state` was hydrated from an older deploy), post-effect hooks fall back to `template.fillWithoutEncode(coerced_params)` — same lossy behavior as before this change, but only on those bypass paths. | ||
|
|
||
| ### SSR consideration | ||
|
|
||
| When the server emits `activity.context.path` (e.g. via `initialContext.req.path` flowing through `historyEntryToEvents`), the client's `onInit` URL-replay trusts the server-emitted path rather than recomputing. This avoids encode-version mismatches between server and client builds. If you upgrade `encode` for a route, redeploy server and client together. | ||
|
|
||
| ### Cross-deploy hydration legacy fallback | ||
|
|
||
| Entries serialized into `history.state` before `stepContext.path` landed on core events have no `step.context.path`. On URL-arrival into such state, `onBeforeStepPush` runs the recompute branch — which requires the parent activity to be present in the stack. During initial boot, the parent might not be materialized yet, so recompute is skipped and post-effect falls back to `fillWithoutEncode(coerced)`. Acceptable as a transitional state across one deploy boundary; subsequent navigations populate `stepContext.path` correctly. | ||
|
|
||
| ## Decision record | ||
|
|
||
| - **Decision:** runtime contract is "always-string at the plugin boundary"; the underlying type declaration is unchanged. | ||
| - **Drivers:** the originating user complaint was about runtime type divergence between in-process navigation and URL arrival, not about wanting typed values in the store. Type-widening would force every consumer to handle non-string runtime types at the usage site. | ||
| - **Alternatives considered:** (a) widen `ActivityBaseParams` to `unknown`; (b) move coercion into `@stackflow/core`. Both rejected — see "Boundary decision" tradeoff above. | ||
| - **Why chosen:** keeps the type contract stable, fixes the runtime divergence at the plugin layer that owns the URL-serialization concern. | ||
| - **Consequences:** programmatic-only consumers (no `historySyncPlugin`) keep typed values in store; the invariant is plugin-conditional. Documented for future maintainers. | ||
| - **Follow-ups:** if a future requirement prefers direction (a) or (b), a new ticket should track that work; this implementation does not pre-empt it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Align activity name in example with the page’s own config naming.
Line 135 uses
push("Article", ...), but this doc consistently uses names likeArticleActivity. Keeping names aligned avoids copy/paste confusion.Suggested patch
🤖 Prompt for AI Agents