Conversation
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Router as Router (hydrate / runtime)
participant SSR as Server (render)
participant Stream as renderRouterToStream
rect rgba(100,150,250,0.5)
Client->>Router: initial load -> request page
Router->>SSR: server render request (renderRouterToString / renderRouterToStream)
SSR->>Stream: render VNode -> renderToStringAsync / renderToStream
Stream->>SSR: buffered HTML / scripts (takeBufferedHtml / takeBufferedScripts)
SSR->>Client: response HTML (with buffered scripts and nonce)
Client->>Router: hydrate (RouterClient.hydrate)
Router->>Router: apply buffered scripts, emit hydration finished
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (16)
examples/preact/basic/README.md-12-12 (1)
12-12:⚠️ Potential issue | 🟡 Minor
gitpickpath points to the React example instead of the Preact example.-npx gitpick TanStack/router/tree/main/examples/react/basic basic +npx gitpick TanStack/router/tree/main/examples/preact/basic basic🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/preact/basic/README.md` at line 12, The README's example clone command points to the React example; update the gitpick path to the Preact example by changing "TanStack/router/tree/main/examples/react/basic" to "TanStack/router/tree/main/examples/preact/basic" so the npx gitpick command pulls the correct Preact example.examples/preact/basic/package.json-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorPackage
nameincorrectly identifies this as a React example.- "name": "tanstack-router-react-example-basic", + "name": "tanstack-router-preact-example-basic",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/preact/basic/package.json` at line 2, The package.json "name" field incorrectly labels this as a React example; update the "name" value in examples/preact/basic/package.json (the "name" property) to reflect Preact instead of React—e.g., change "tanstack-router-react-example-basic" to something like "tanstack-router-preact-example-basic" (or another Preact-specific identifier) so the package name accurately represents the Preact example.packages/preact-router/src/ClientOnly.tsx-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorFix import sort order per ESLint.
ESLint reports that
useEffectshould precedeuseStatealphabetically.Proposed fix
-import { useState, useEffect } from 'preact/hooks' +import { useEffect, useState } from 'preact/hooks'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/ClientOnly.tsx` at line 2, The import statement in ClientOnly.tsx is failing ESLint's sort order because useEffect should come before useState alphabetically; update the import from 'preact/hooks' so that the named import list is alphabetized (useEffect, useState) to satisfy the linter and keep the import sorted.packages/preact-router/tests/ClientOnly.test.tsx-21-34 (1)
21-34:⚠️ Potential issue | 🟡 MinorTest doesn't verify what its name claims.
The test is named "renders fallback initially" but never asserts that
'Fallback'text appears. Since@testing-library/preactin jsdom flushes effects synchronously, the assertion on line 33 only checks the post-hydration state ('Client Content'), making this test functionally identical to the third test.To actually test the fallback, you'd need to mock or suppress
useEffectto observe the pre-hydration render. Otherwise, consider renaming this test to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/ClientOnly.test.tsx` around lines 21 - 34, The test named "renders fallback initially" is misleading because it never asserts the fallback and effects run synchronously; either change the test to actually assert the pre-hydration fallback by preventing useEffect from running (e.g., mock/suppress React/Preact's useEffect inside the test or wrap ClientOnly in a stub that disables hydration effects) and then assert that container.textContent contains 'Fallback', or simply rename the test to something like "renders client content after hydration" to match the existing assertion; locate the ClientOnly usage in the test and update the test setup or name accordingly.packages/preact-router/src/Match.tsx-214-241 (1)
214-241:⚠️ Potential issue | 🟡 MinorThrowing
undefinedis possible ifrouter.getMatch()returnsundefined.Lines 215, 219, and 240 all use optional chaining:
router.getMatch(match.id)?._nonReactive.loadPromise. IfgetMatchreturnsundefined,throw undefinedwould propagate to the error boundary rather than suspending. While this shouldn't happen in practice (the match was just selected from state), this is a subtle failure mode.A defensive
invariantor fallback before the throw would make this more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/Match.tsx` around lines 214 - 241, The code may throw undefined when calling router.getMatch(match.id)?._nonReactive.*; fix by eagerly retrieving const routerMatch = router.getMatch(match.id) before any throw and add a defensive check (e.g., invariant or throw new Error) if routerMatch is undefined, then use routerMatch._nonReactive.displayPendingPromise, .minPendingPromise or .loadPromise for the throws and for setting minPendingPromise; update the branches around match._displayPending, match._forcePending and match.status === 'pending' to use this guarded routerMatch variable.packages/preact-router/src/link.tsx-221-222 (1)
221-222:⚠️ Potential issue | 🟡 Minor
includeHashcheck returnsfalseunconditionally on the server path when hash is requested.Line 221:
if (activeOptions?.includeHash) return false— When the user setsincludeHash: true, the server SSR path always treats the link as inactive because hash comparison isn't performed. The client path (line 401-403) at least comparess.location.hash === next.hash. Consider adding a comment clarifying this is intentional SSR behavior, or performing the hash comparison here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/link.tsx` around lines 221 - 222, The server-side branch unconditionally returns false when activeOptions?.includeHash is true, which makes SSR always treat hash-targeted links as inactive; update the server path in link.tsx so that instead of immediately returning false it either (A) documents this limitation with a clear comment stating includeHash cannot be evaluated during SSR, or (B) performs a hash comparison similar to the client path by checking available hash values (mirror the client check s.location.hash === next.hash against next.hash) when a current hash value is present; modify the conditional using activeOptions?.includeHash to either add the explanatory comment or replace the unconditional return with a guarded comparison to correctly determine active state when a hash is available.packages/preact-router/tests/link.test.tsx-12-15 (1)
12-15:⚠️ Potential issue | 🟡 MinorRemove unused import and mock variables.
getIntersectionObserverMock(line 12),ioObserveMock(line 14), andioDisconnectMock(line 15) are defined but never used in any test. ESLint also flags the unused import.Proposed fix
import { Link, Outlet, RouterProvider, createMemoryHistory, createRootRoute, createRoute, createRouter, } from '../src' -import { getIntersectionObserverMock } from './utils' - -const ioObserveMock = vi.fn() -const ioDisconnectMock = vi.fn()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/link.test.tsx` around lines 12 - 15, Remove the unused import and mocks: delete the getIntersectionObserverMock import and the two unused mock constants ioObserveMock and ioDisconnectMock from the tests so they are not declared but unused; locate the declarations by the symbol names getIntersectionObserverMock, ioObserveMock, and ioDisconnectMock (in link.test.tsx) and remove those lines, or alternatively, if they were intended to be used, wire them into the tests (e.g., pass them to the Intersection Observer mock setup) — otherwise simply remove the import and const declarations.e2e/preact-router/basic/tests/app.spec.ts-11-12 (1)
11-12:⚠️ Potential issue | 🟡 MinorPotential timing race between
slow-titlevisible andsuspense-fallbackassertion.The slow route resolves its Suspense content after a short fixed delay (200 ms per the PR description). Playwright's
toBeVisible()polls the DOM until the condition is met or the timeout is reached — it does not succeed retroactively if the element already disappeared. If the time between line 11 (slow-titlevisible) and line 12 (suspense-fallbackvisible) exceeds that 200 ms delay on a slow/loaded CI runner, the fallback will have already been replaced, causing line 12 to time out and fail.The safest fix is to increase the suspense delay in the test app (e.g., 2 000 ms instead of 200 ms), giving Playwright ample time to assert the fallback state before it resolves.
🛡️ Suggestion: increase delay in the slow route for test reliability
In
e2e/preact-router/basic/src/main.tsx(or wherever the slow route is defined), increase the artificial delay:-await new Promise((resolve) => setTimeout(resolve, 200)) +await new Promise((resolve) => setTimeout(resolve, 2000))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/preact-router/basic/tests/app.spec.ts` around lines 11 - 12, The test can race because the slow route's Suspense fallback may disappear before Playwright asserts it; update the slow route's artificial delay (where the slow route is defined, e.g., in e2e/preact-router/basic/src/main.tsx or the slow route component) to a much larger value (e.g., increase from 200 ms to 2000 ms) so the fallback with testIds 'suspense-fallback' and 'slow-title' remains present long enough for both expects to reliably assert visibility.packages/preact-router/tests/useParams.test.tsx-1-4 (1)
1-4:⚠️ Potential issue | 🟡 MinorRemove three unused imports:
vi,fireEvent, andLinkAll three are flagged by static analysis and are absent from the test body.
🔧 Proposed fix
-import { afterEach, expect, test, vi } from 'vitest' -import { act, cleanup, fireEvent, render, screen } from '@testing-library/preact' +import { afterEach, expect, test } from 'vitest' +import { act, cleanup, render, screen } from '@testing-library/preact' import { - Link, Outlet, RouterProvider,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/useParams.test.tsx` around lines 1 - 4, The import list includes three unused symbols — vi, fireEvent, and Link — which should be removed to satisfy static analysis; update the top imports so only used identifiers remain (e.g., keep afterEach, expect, test from 'vitest' and act, cleanup, render, screen from '@testing-library/preact'), save the file, and re-run the tests/linter to confirm the unused-import warnings are gone.packages/preact-router/tests/router.test.tsx-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorFix misplaced
Outletimport and remove unusedviThree ESLint errors (flagged by static analysis) stem from the same root cause —
Outletwas added at the bottom of the file rather than merged into the existing'../src'import block, andviwas imported without being used.🔧 Proposed fix
-import { afterEach, describe, expect, test, vi } from 'vitest' +import { afterEach, describe, expect, test } from 'vitest' import { cleanup, render, screen } from '@testing-library/preact' import { + Outlet, RouterProvider, createMemoryHistory, createRootRoute, createRoute, createRouter, } from '../src'-// Need Outlet import -import { Outlet } from '../src'Also applies to: 109-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/router.test.tsx` at line 1, Move the misplaced Outlet import into the existing import block that imports from '../src' (i.e., merge the separate Outlet import into that module import) and remove the unused vi identifier from the test imports (the import line importing afterEach, describe, expect, test, vi should drop vi). Update the import statements so Outlet is imported alongside other exported symbols from '../src' and ensure no unused vi import remains.packages/preact-router/src/ssr/RouterClient.tsx-6-21 (1)
6-21:⚠️ Potential issue | 🟡 MinorTypeScript strict mode reports a type error:
hydrationPromisemay beundefinedwhen passed toAwaitThe
Awaitcomponent requires itspromiseprop to be typed asPromise<T>(non-optional). After theif (!hydrationPromise)block, both branches assign aPromise, but TypeScript does not narrow module-levelletvariables (they can be mutated elsewhere in the module), sohydrationPromiseretains the typePromise<void | Array<Array<void>>> | undefined. This is incompatible with the non-optionalPromise<T>type expected byAwaitin strict mode.The fix is either a non-null assertion or a local const:
🛡️ Proposed fixes
- promise={hydrationPromise} + promise={hydrationPromise!}Or:
+ const promise = hydrationPromise return ( <Await - promise={hydrationPromise} + promise={promise} children={() => <RouterProvider router={props.router} />} /> )Secondary note — test-isolation: The module-level
hydrationPromiseis intentional to ensure a single hydration per page load, but any test renderingRouterClientdirectly will share this promise across test cases. If unit/integration tests exerciseRouterClient, ensure the module-level variable is reset between tests (e.g., viavi.resetModules()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/ssr/RouterClient.tsx` around lines 6 - 21, The module-level hydrationPromise variable can be undefined from TypeScript's perspective when passed to Await; fix by creating a local non-optional promise before rendering and pass that to Await — e.g. inside RouterClient after the existing if-block declare a const promise = hydrationPromise! (or const promise = hydrationPromise ?? Promise.resolve()) and pass promise to <Await> instead of hydrationPromise; reference symbols: hydrationPromise, RouterClient, Await, hydrate, RouterProvider.packages/preact-router/tests/loaders.test.tsx-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove unused
actimport.
actis imported but never used — ESLint flags this asunused-imports/no-unused-imports.🛠️ Proposed fix
-import { act, cleanup, fireEvent, render, screen } from '@testing-library/preact' +import { cleanup, fireEvent, render, screen } from '@testing-library/preact'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/loaders.test.tsx` at line 2, The import list in the test currently includes an unused symbol `act`; update the top-level import from '@testing-library/preact' to remove `act` so only used utilities (`cleanup`, `fireEvent`, `render`, `screen`) are imported, ensuring no other references to `act` remain in `loaders.test.tsx` (or any helper functions) before committing the change.e2e/preact-router/basic/playwright.config.ts-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorRemove unnecessary escape characters around
"test".Inside a template literal,
"does not need escaping. The\"sequences produce"at runtime (same result), but the ESLintno-useless-escapeerror is valid and the redundant escapes are misleading.🛠️ Proposed fix
- command: `VITE_NODE_ENV=\"test\" VITE_SERVER_PORT=${PORT} VITE_EXTERNAL_PORT=${EXTERNAL_PORT} pnpm build && pnpm preview --port ${PORT}`, + command: `VITE_NODE_ENV="test" VITE_SERVER_PORT=${PORT} VITE_EXTERNAL_PORT=${EXTERNAL_PORT} pnpm build && pnpm preview --port ${PORT}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/preact-router/basic/playwright.config.ts` at line 22, The template-literal string assigned to the command variable contains unnecessary escaped quotes around "test" (VITE_NODE_ENV=\"test\"); remove the backslashes so the value is plain "test" inside the template literal (VITE_NODE_ENV="test") to satisfy ESLint no-useless-escape—update the command assignment in the Playwright config (the line defining command) accordingly.packages/preact-router/tests/useBlocker.test.tsx-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorRemove unused
viimport.
viis imported but never referenced — ESLint correctly flags this asunused-imports/no-unused-imports.🛠️ Proposed fix
-import { afterEach, describe, expect, test, vi } from 'vitest' +import { afterEach, describe, expect, test } from 'vitest'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/useBlocker.test.tsx` at line 1, The import list from 'vitest' includes an unused symbol `vi`; remove `vi` from the named imports in the import statement so only used symbols (afterEach, describe, expect, test) are imported, and run tests/lint to confirm no other references to `vi` remain (if any mocking was intended, switch to the global `vi` instead of importing it).packages/preact-router/tests/ssr.test.tsx-49-76 (1)
49-76:⚠️ Potential issue | 🟡 Minor
renderRouterToStreamtest is missingsetRenderFinishedassertion and has a weaker doctype check.Two observations on this test:
The
renderRouterToStreamimplementation (inrenderRouterToStream.tsxline 16) callsrouter.serverSsr!.setRenderFinished(), just likerenderRouterToString. The first test asserts this (line 45) but this test does not. Add the missing assertion for parity.Line 72 asserts
toContain('<html>')while test 1 (line 42) assertstoContain('<!DOCTYPE html>'). Both implementations prepend<!DOCTYPE html>, so the weaker check here could mask a regression.Suggested fix
expect(response.status).toBe(200) - expect(html).toContain('<html>') + expect(html).toContain('<!DOCTYPE html>') expect(html).toContain('Hello Stream SSR') expect(html).toContain('<script>window.__SSR__=1</script>') + expect(router.serverSsr.setRenderFinished).toHaveBeenCalledTimes(1) expect(router.serverSsr.cleanup).toHaveBeenCalledTimes(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/ssr.test.tsx` around lines 49 - 76, Add the missing assertion that router.serverSsr.setRenderFinished was called and make the doctype check strict: in the renderRouterToStream test update the expectations to assert expect(router.serverSsr.setRenderFinished).toHaveBeenCalledTimes(1) (matching the renderRouterToString test) and replace the weaker expect(html).toContain('<html>') with expect(html).toContain('<!DOCTYPE html>') so the test verifies the explicit doctype and parity with the other test.packages/preact-router/src/Matches.tsx-248-259 (1)
248-259:⚠️ Potential issue | 🟡 Minor
useParentMatches/useChildMatchesbehave unexpectedly whencontextMatchIdis undefined.If these hooks are called outside a
matchContext.Provider(wherecontextMatchIdisundefined),findIndexreturns-1, causingslice(0, -1)to return all matches except the last instead of an empty array. A similar issue exists inuseChildMatchesat line 278, whereslice(0)returns all matches.This is unlikely to cause issues in practice since these hooks should only be called within a rendered route tree, but a guard could make it more predictable:
🛡️ Optional defensive guard
export function useParentMatches<...>(...): ... { const contextMatchId = useContext(matchContext) return useMatches({ select: (matches: Array<MakeRouteMatchUnion<TRouter>>) => { + const index = matches.findIndex((d) => d.id === contextMatchId) matches = matches.slice( 0, - matches.findIndex((d) => d.id === contextMatchId), + index === -1 ? 0 : index, ) return opts?.select ? opts.select(matches) : matches },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/Matches.tsx` around lines 248 - 259, When contextMatchId is undefined the select callback's findIndex returns -1 causing slice to return the wrong subset; update the select in useParentMatches and the analogous select in useChildMatches to first check for contextMatchId (or that findIndex returned -1) and return an empty array (or appropriate empty slice) instead of slicing with -1 — locate the select callbacks in useParentMatches/useChildMatches, compute const idx = matches.findIndex(d => d.id === contextMatchId), and if idx === -1 return [] (or the empty result expected by that hook) otherwise return opts?.select ? opts.select(matches.slice(0, idx)) : matches.slice(0, idx).
🧹 Nitpick comments (23)
packages/preact-router/src/Transitioner.tsx (1)
32-37:isTransitioningis effectively alwaysfalsedue to synchronous batching in Preact.Unlike React's Transitioner (which wraps the call in
React.startTransition()to prevent batching), Preact's synchronoussetIsTransitioning(true)andsetIsTransitioning(false)calls on lines 34–36 are batched together in a single render cycle. This meansisAnyPending(line 26) effectively always reduces toisLoading || hasPendingMatches, making theisTransitioningstate unused and potentially confusing for maintainers.The comment on line 32 acknowledges this limitation. Consider either removing
isTransitioningentirely or adding a brief comment at line 15 noting it's a no-op placeholder for parity with the React version.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/Transitioner.tsx` around lines 32 - 37, isTransitioning is effectively a no-op in this Preact implementation because router.startTransition sets setIsTransitioning(true) and immediately setIsTransitioning(false), so the state never persists; update the code to either remove isTransitioning and all references (including the isTransitioning state and the router.startTransition implementation) or clearly mark it as a deliberate no-op for parity with React by adding a concise comment near the top where isTransitioning is declared and leaving router.startTransition as a placeholder; ensure you update any derived boolean (isAnyPending) to reflect removal (use isLoading || hasPendingMatches) if you remove isTransitioning, and remove any unused imports or variables tied to isTransitioning.examples/preact/basic/tsconfig.json (1)
5-6: Mixed indentation: lines 5–6 use tabs while the rest of the file uses spaces.🧹 Suggested fix
- "jsx": "react-jsx", - "jsxImportSource": "preact", + "jsx": "react-jsx", + "jsxImportSource": "preact",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/preact/basic/tsconfig.json` around lines 5 - 6, Lines containing the "jsx" and "jsxImportSource" properties use tabs while the rest of the tsconfig.json uses spaces; replace the tabs on the "jsx" and "jsxImportSource" lines with the same space-based indentation used elsewhere so the file indentation is consistent and no mixed tabs/spaces remain.packages/preact-router/tests/setupTests.tsx (1)
5-5: Consider adding a type cast for strict-mode compatibility.
vi.fn()returnsMock<[], void>, which is not assignable to the overloadedWindow['scrollTo']signature under strict TypeScript. This may surface as a type error depending on the package's tsconfig coverage for test files.🛡️ Suggested fix
-window.scrollTo = vi.fn() +window.scrollTo = vi.fn() as unknown as typeof window.scrollTo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/setupTests.tsx` at line 5, Assign a properly typed mock to window.scrollTo to satisfy strict TypeScript: replace the plain vi.fn() assignment by casting the mock (vi.fn()) to the Window['scrollTo'] type (or via unknown then to Window['scrollTo']) so the mocked function signature matches window.scrollTo; update the code where window.scrollTo is set and reference vi.fn() and window.scrollTo in the change.pnpm-workspace.yaml (1)
15-17:e2e/preact-router/*is ordered aftere2e/react-router/*, breaking alphabetical consistency with the rest of thee2e/entries.🧹 Suggested fix
+ - 'e2e/preact-router/*' - 'e2e/react-router/*' - - 'e2e/preact-router/*' - 'e2e/solid-router/*'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pnpm-workspace.yaml` around lines 15 - 17, The e2e entries in pnpm-workspace.yaml are out of alphabetical order: 'e2e/preact-router/*' should come before 'e2e/react-router/*'. Edit the list so the three entries are alphabetically ordered (place 'e2e/preact-router/*' above 'e2e/react-router/*', keeping 'e2e/solid-router/*' after them) to restore consistency.examples/preact/basic/package.json (1)
11-18:@tailwindcss/viteandtailwindcssbelong indevDependencies.Both are build-time tools — they produce no runtime output. Listing them under
dependencieswould unnecessarily inflatenode_modulesfor consumers doing a production install.preact-render-to-string(line 15) is an SSR library; if this example is client-side only, it too should be indevDependenciesor removed.🛠️ Suggested fix
"dependencies": { - "@tailwindcss/vite": "^4.1.18", "@tanstack/preact-router": "^1.160.0", "preact": "^10.25.0", - "preact-render-to-string": "^6.6.5", "redaxios": "^0.5.1", - "tailwindcss": "^4.1.18" }, "devDependencies": { + "@tailwindcss/vite": "^4.1.18", "@preact/preset-vite": "^2.10.3", + "preact-render-to-string": "^6.6.5", + "tailwindcss": "^4.1.18", "typescript": "^5.7.2", "vite": "^7.3.1" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/preact/basic/package.json` around lines 11 - 18, Move build-only packages out of the dependencies block: remove "@tailwindcss/vite" and "tailwindcss" from the "dependencies" object and add them to "devDependencies" instead; also evaluate "preact-render-to-string" (SSR-only) — either move it to "devDependencies" or remove it if this example is strictly client-side; update the package.json so runtime "dependencies" only contain actual runtime libs like "preact", "@tanstack/preact-router", and "redaxios".packages/preact-router/tests/navigate.test.tsx (1)
84-87:anyannotation on the param function can be narrowed.The
(p: any)on line 86 opts out of type checking on the existing params object. Since strict TypeScript is required for this project, consider using the router's inferredParamstype ortypeof routerutilities instead.♻️ Suggested improvement
- params: (p: any) => ({ ...p, slug: 'tkdodo' }), + params: (p) => ({ ...p, slug: 'tkdodo' }),Removing the explicit annotation lets TypeScript infer the param type from the route definition, which is safer and keeps the test in strict mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/navigate.test.tsx` around lines 84 - 87, The params callback passed to router.navigate currently uses an explicit any annotation `(p: any)` which disables strict typing; remove the explicit annotation so TypeScript infers the correct params type from the router (or replace with the router's inferred Params utility if you prefer explicit typing), e.g., update the params callback in the router.navigate call so the parameter is untyped (let inference handle it) or typed with the router's Params type instead of any to restore strict type checking.examples/preact/basic/src/styles.css (1)
3-18: Minor redundancy: border color set in both@layer baseand the bare*rule.The
@layer baseblock at lines 4–10 setsborder-color: var(--color-gray-200, currentcolor)on*and all pseudo-elements. The bare* {@applyborder-gray-200 dark:border-gray-800; }at line 16 re-applies the same light-mode color to*(outside a layer, so it wins the cascade). The@layer baseblock is still needed to cover pseudo-elements, but for*itself the two declarations are redundant in light mode.♻️ Suggested consolidation
`@layer` base { *, ::after, ::before, ::backdrop, ::file-selector-button { border-color: var(--color-gray-200, currentcolor); } + `@media` (prefers-color-scheme: dark) { + * { + border-color: var(--color-gray-800, currentcolor); + } + } } html { color-scheme: light dark; } -* { - `@apply` border-gray-200 dark:border-gray-800; -} body { `@apply` bg-gray-50 text-gray-950 dark:bg-gray-900 dark:text-gray-200; }Alternatively, keep the
@applyapproach and drop the@layer baseif pseudo-element coverage isn't needed for this example.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/preact/basic/src/styles.css` around lines 3 - 18, The rules set border color twice for the universal selector: once inside the `@layer` base block (selectors: *, ::after, ::before, ::backdrop, ::file-selector-button) and again in the bare universal rule (* { `@apply` border-gray-200 dark:border-gray-800; }), causing redundancy; remove the duplicate for the universal selector by keeping the `@layer` base rule only for pseudo-elements and adjusting it to exclude the bare * (or conversely delete the bare * rule and explicitly add pseudo-element selectors inside `@layer` base), ensuring pseudo-elements still receive border-color while the universal selector is defined in one place (refer to the selectors and the bare '*' rule to locate where to change).packages/preact-router/tests/utils.ts (1)
31-38:threshold→thresholdstype coercion is masked byas any.
IntersectionObserverInit.thresholdisnumber | number[], butIntersectionObserver.thresholdsisReadonlyArray<number>. Whenthresholdis a singlenumber, this assigns anumberto an array-typed field. Since this is test-only code the impact is minimal, but a proper normalization would be safer.Suggested fix
- this.thresholds = options?.threshold ?? ([0] as any) + const t = options?.threshold ?? [0] + this.thresholds = Array.isArray(t) ? t : [t]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/utils.ts` around lines 31 - 38, The constructor currently coerces IntersectionObserverInit.threshold into this.thresholds using "as any", which can assign a single number to thresholds (should be ReadonlyArray<number>); update the normalization in the constructor so if options?.threshold is a number convert it to an array [number], if it's already an array use it as-is, and default to [0] when undefined; modify the assignments around this.root, this.rootMargin, and this.thresholds in the constructor to perform this safe normalization for IntersectionObserverInit.threshold.packages/preact-router/src/routerContext.tsx (1)
21-21: Remove unnecessaryas anycast.
routerContextisContext<AnyRouter>andwindow.__TSR_ROUTER_CONTEXT__is declared asReturnType<typeof createContext<AnyRouter>>(alsoContext<AnyRouter>). These types are directly assignable without the cast.Proposed fix
- window.__TSR_ROUTER_CONTEXT__ = routerContext as any + window.__TSR_ROUTER_CONTEXT__ = routerContext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/routerContext.tsx` at line 21, Remove the unnecessary type assertion when assigning the router context to the global: replace the `window.__TSR_ROUTER_CONTEXT__ = routerContext as any` assignment with a direct assignment since `routerContext` is already `Context<AnyRouter>` and matches `ReturnType<typeof createContext<AnyRouter>>`; update the single assignment site referencing `routerContext` and `window.__TSR_ROUTER_CONTEXT__` to drop the `as any` cast.packages/preact-router/package.json (2)
66-68: Node engine minimum of 12 is extremely dated.Node 12 has been EOL since April 2022. If this is a project-wide convention, it's fine to keep for consistency, but worth noting that modern Node features (e.g., ESM support,
structuredClone) may not be available at this floor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/package.json` around lines 66 - 68, Update the package.json engines policy to a modern Node LTS baseline by changing the "engines" -> "node" value from ">=12" to a current supported minimum (for example ">=16" or ">=18"); edit the "engines" object in package.json (the "node" property) to the chosen newer version, run CI/test to ensure compatibility, and if this is a repo-wide convention coordinate with other packages to keep the same minimum across the project.
69-77: Consider making SSR-only dependencies optional or peer dependencies.
preact-render-to-stringandisbotare only needed by the./ssr/serverentrypoint, but they're listed as unconditional runtimedependencies. Consumers who only use client-side routing will still install these. Consider moving them topeerDependencieswithpeerDependenciesMetamarking them as optional, or documenting this trade-off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/package.json` around lines 69 - 77, The package lists SSR-only libs ("preact-render-to-string" and "isbot") under dependencies causing clients to install them unnecessarily; update package.json to move these two packages out of "dependencies" into "peerDependencies" and add a "peerDependenciesMeta" entry marking them as optional (e.g., "preact-render-to-string": { "optional": true }, "isbot": { "optional": true }) so the ./ssr/server entrypoint can require them only when present; alternatively, if you prefer consumers not to provide them, move them to "optionalDependencies" or document the SSR-only requirement in the README for the ./ssr/server entrypoint.packages/preact-router/src/useNavigate.tsx (1)
54-59: Props reference check is always true — navigation fires on every re-render.
previousPropsRef.current !== propsis a reference-identity comparison, but Preact (like React) creates a newpropsobject on every render. Combined withpropsin the effect's dependency array, this meansnavigate(props)runs on every re-render of the parent component, not just when the navigation target changes.This mirrors the React router's
Navigateimplementation and works in practice because the component is typically used for one-shot redirects that unmount quickly. If this is intentional, a brief comment explaining the rationale would help future readers. Otherwise, a shallow comparison of the navigation-relevant fields would be more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/useNavigate.tsx` around lines 54 - 59, The effect in useLayoutEffect currently compares previousPropsRef.current !== props using reference identity, causing navigate(props) to run every render; change this to a shallow equality check of the navigation-relevant fields (e.g., compare previousPropsRef.current.to, .replace, .state or whatever props are used by navigate) before calling navigate, and update previousPropsRef.current only when those relevant fields differ; alternatively, if one-shot redirects are intended, add a concise comment above the useLayoutEffect explaining that reference inequality is expected and intentional to avoid future confusion.packages/preact-router/src/CatchBoundary.tsx (1)
80-119:ErrorComponentsilently swallows non-Errorthrown values.When
erroris not anErrorinstance (e.g., a thrown string or object without.message),error.messageisundefinedand the<pre>renders empty. Consider a fallback likeString(error)to surface something useful.Proposed fallback for non-Error values
- {error.message ? <code>{error.message}</code> : null} + <code>{error.message ?? String(error)}</code>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/CatchBoundary.tsx` around lines 80 - 119, ErrorComponent currently only renders error.message and therefore shows nothing for non-Error throws; update the rendering logic in ErrorComponent (the error prop handling) to fall back to a stringified representation when error.message is falsy: if error is an object use JSON.stringify(error, null, 2) (with try/catch to avoid circular errors) otherwise use String(error) or 'Unknown error' for null/undefined, and render that inside the existing <pre>/<code> area so non-Error values are visible.packages/preact-router/src/useBlocker.tsx (2)
263-285:_resolvePromptBlockerArgslargely duplicates_resolveBlockerOpts.The legacy-options conversion logic (lines 270-284) is nearly identical to lines 115-129 of
_resolveBlockerOpts. Consider extracting the shared legacy-to-opts conversion into a common helper to reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/useBlocker.tsx` around lines 263 - 285, The logic in _resolvePromptBlockerArgs that converts legacy props (condition/blockerFn) into UseBlockerOpts duplicates _resolveBlockerOpts; extract that shared conversion into a single helper (e.g., normalizeLegacyPromptProps or legacyToUseBlockerOpts) which takes a PromptProps | LegacyPromptProps and returns { shouldBlockFn, enableBeforeUnload, withResolver } by computing shouldBlock = Boolean(props.condition ?? true), wrapping blockerFn into _customBlockerFn and setting enableBeforeUnload and withResolver accordingly; then replace the duplicated block in both _resolvePromptBlockerArgs and _resolveBlockerOpts to call the new helper and return its result.
224-245: Misleading variable namecanNavigateAsync.
canNavigateAsyncholdstruewhen navigation should be blocked (fromreset) andfalsewhen it should proceed. The name suggests the opposite. Consider renaming toshouldBlockResultorshouldStayOnPagefor clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/useBlocker.tsx` around lines 224 - 245, The variable canNavigateAsync is misnamed because it is true when navigation should be blocked (reset) and false when it should proceed; rename it to a clear name like shouldStayOnPage or shouldBlock to reflect that truthy means "stay" and update the return to match the new name; update references in this function (the Promise resolver logic using proceed/reset and the setResolver call) and any callers expecting the old boolean semantics so behavior remains unchanged after the rename.packages/preact-router/src/router.ts (1)
73-79:windowfallback branch is unreachable.
globalThisis defined in all modern JS environments (browsers, Node.js, Deno, workers). Theelse if (typeof window !== 'undefined')branch on line 76 is effectively dead code. This mirrors the React router's pattern, so it may be intentional for legacy compatibility, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/router.ts` around lines 73 - 79, The fallback branch checking window is unreachable because globalThis exists in modern environments; remove the redundant "else if (typeof window !== 'undefined')" block and keep only the globalThis assignments for createFileRoute and createLazyFileRoute (or alternatively convert the else-if into a separate independent if that also assigns to window if you need legacy window exposure), referencing the globalThis and window symbols and the createFileRoute/createLazyFileRoute assignments to locate the code to change.packages/preact-router/src/HeadContent.tsx (1)
16-16:JSON.stringify(tag)as a key is called on every render for every tag.The entire tag object (including
attrsandchildren) is serialized to compute the React/Preact key on every render pass. For routes with inline scripts or large style blobs, this serialization is non-trivial. A more targeted stable key (e.g., combiningtag.tagwith a discriminating attribute likehref,src,name, or a hash) would avoid repeated full-object serialization.♻️ Example of a leaner key strategy
-<Asset {...tag} key={`tsr-meta-${JSON.stringify(tag)}`} nonce={nonce} /> +<Asset + {...tag} + key={`tsr-meta-${tag.tag}-${ + tag.attrs + ? (tag.attrs.href ?? + tag.attrs.src ?? + tag.attrs.name ?? + tag.attrs.property ?? + tag.attrs.rel ?? + JSON.stringify(tag.attrs)) + : tag.children + }`} + nonce={nonce} +/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/HeadContent.tsx` at line 16, The current key uses JSON.stringify(tag) which serializes the whole tag object on every render; update the key generation in HeadContent (where Asset is rendered with props {...tag} and nonce) to use a stable, lightweight discriminator such as `${tag.tag}-${tag.attrs?.href||tag.attrs?.src||tag.attrs?.name||tag.attrs?.id||index}` or, if you need collision resistance, compute a small hash only over the relevant attrs (not children) and use that instead; ensure you reference Asset and the local variable tag (and fallback to the loop index) so the key is deterministic, cheap to compute, and avoids serializing large children blobs.packages/preact-router/tests/loaders.test.tsx (1)
129-133:expectinside loader function obscures assertion failure messages.When
expect(parentMatch.loaderData).toBe('nested')fails inside the loader, the thrown error is caught by the router's loader error handling rather than propagating directly to Vitest. The test then fails with afindByTexttimeout instead of the original assertion message, making failures hard to diagnose. Consider asserting after the route resolves instead:♻️ Suggested refactor
const fooRoute = createRoute({ getParentRoute: () => nestedRoute, path: '/foo', loader: async ({ parentMatchPromise }) => { nestedLoaderMock(parentMatchPromise) - const parentMatch = await parentMatchPromise - expect(parentMatch.loaderData).toBe('nested') + return parentMatchPromise }, component: () => <div>Nested Foo page</div>, }) // ... const fooElement = await screen.findByText('Nested Foo page') expect(fooElement).toBeInTheDocument() + const resolvedParent = await nestedLoaderMock.mock.calls[0]?.[0] + expect(resolvedParent).toBeDefined() + expect((await resolvedParent).loaderData).toBe('nested') expect(nestedLoaderMock).toHaveBeenCalled() expect(nestedLoaderMock.mock.calls[0]?.[0]).toBeInstanceOf(Promise)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/loaders.test.tsx` around lines 129 - 133, The assertion inside the async loader is swallowed by the router's loader error handling; move the check out of the loader and into the test after the route has resolved: have the loader call nestedLoaderMock(parentMatchPromise) and await parentMatchPromise inside the test (or wait for the rendered route via findByText), then assert parentMatch.loaderData === 'nested' in the test body rather than inside the loader function so Vitest receives the actual assertion failure; reference the loader block using the loader: async ({ parentMatchPromise }) => { ... } and the nestedLoaderMock(parentMatchPromise) call to locate where to remove the inline expect and place the assertion after resolution.packages/preact-router/src/ssr/renderRouterToStream.tsx (1)
5-16:renderRouterToStreamis named for streaming but returns a fully-bufferedResponse.The implementation uses
renderToStringAsync(which resolves all promises into a single string) and wraps it in a non-streamingnew Response(...). This is behaviorally identical torenderRouterToStringand does not produce an incremental/chunked response. Consumers expecting a true streaming response would be misled.Given the PR notes acknowledge this ("not battle-tested"), consider renaming to
renderRouterToStringAsyncor adding a clear// NOTE: not a true streaming responseJSDoc comment until actual streaming is implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/ssr/renderRouterToStream.tsx` around lines 5 - 16, The function renderRouterToStream is misnamed because it fully buffers output using renderToStringAsync and returns a non-streaming Response; rename the exported function to renderRouterToStringAsync (update the function declaration and export symbol renderRouterToStream -> renderRouterToStringAsync) and update all internal references/imports/tests/docs that call renderRouterToStream; alternatively, if you prefer to keep the name, add a clear JSDoc above renderRouterToStream stating "// NOTE: not a true streaming response — returns a fully-buffered Response via renderToStringAsync" and ensure the export and any consumers are aware of the non-streaming behavior (adjust tests/docs accordingly).packages/preact-router/src/headContentUtils.tsx (1)
60-66: Nonce is unnecessarily added to<meta>tag attributes.
nonceis a valid attribute forlink,style, andscriptelements per the CSP spec, but not meaningful on<meta>tags. This won't break anything but emits unnecessary markup.🔧 Remove nonce from meta attrs
resultMeta.push({ tag: 'meta', attrs: { ...m, - nonce, }, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/headContentUtils.tsx` around lines 60 - 66, In headContentUtils.tsx the code that builds resultMeta pushes meta entries with attrs: { ...m, nonce }, which adds a nonce attribute to <meta> tags unnecessarily; update the construction of resultMeta (the object pushed in the resultMeta array where tag === 'meta' and attrs are built) to omit nonce for meta tags (remove adding the nonce property or conditionally include nonce only for tag values 'script', 'style', or 'link') so meta elements no longer receive the nonce attribute.packages/preact-router/tests/ssr.test.tsx (1)
1-77:renderRouterToStreamandrenderRouterToStringhave identical implementations — neither actually implements streaming.Both functions use
renderToStringAsyncto produce a complete HTML string and return a fully-renderedResponse. Therequestparameter inrenderRouterToStreamis unused. Since the implementations are functionally identical, tests won't catch if they diverge or if streaming support is later added to one but not the other.Either document that both currently delegate to string-based rendering, or consolidate them until true streaming support is added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/tests/ssr.test.tsx` around lines 1 - 77, renderRouterToStream duplicates renderRouterToString by producing a full string instead of streaming; fix by consolidating implementations so only one real renderer exists: have renderRouterToStream delegate to renderRouterToString (or vice‑versa) and forward the request/params (or remove the unused request param) so there’s a single source of truth, and add a TODO comment noting true streaming is not yet implemented in the chosen function (referencing renderRouterToStream and renderRouterToString).packages/preact-router/src/Scripts.tsx (1)
48-55: RemovesuppressHydrationWarning—it's a React-only prop that gets filtered out in Preact anyway.
suppressHydrationWarningis a React-specific JSX prop that has no meaning in Preact. While the PreactAsset.tsxcomponent (lines 82, 130) explicitly filters it out before applying attributes to DOM elements, including it here is an unnecessary artifact from the React implementation and creates confusion about intent.Suggested fix
.map(({ children, ...script }) => ({ tag: 'script', attrs: { ...script, - suppressHydrationWarning: true, nonce, }, children, })),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/Scripts.tsx` around lines 48 - 55, In Scripts.tsx remove the React-only prop suppressHydrationWarning from the attrs object created in the map that builds script elements (the block mapping children,...script => ({ tag: 'script', attrs: { ...script, suppressHydrationWarning: true, nonce }, children })). Instead only spread the script props and nonce into attrs; rely on Asset.tsx's existing filtering of suppressHydrationWarning when normalizing DOM attributes so Preact consumers don't see React-specific props and to avoid confusion.packages/preact-router/src/route.tsx (1)
267-316: Consider extracting shared hook wiring to reduce duplication acrossRouteApi,Route, andRootRoute.
RouteApi(lines 108-168),Route(lines 267-316), andRootRoute(lines 525-574) each implement nearly identical sets of 8 hook-method bindings (useMatch,useRouteContext,useSearch,useParams,useLoaderDeps,useLoaderData,useNavigate,Link). A shared helper or mixin could centralize this, reducing maintenance surface and risk of subtle divergence (like thestrict: falsedifference noted above).This is understandable for an initial port — just flagging for future consolidation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/route.tsx` around lines 267 - 316, The RouteApi, Route, and RootRoute classes duplicate the same 8 hook bindings (useMatch, useRouteContext, useSearch, useParams, useLoaderDeps, useLoaderData, useNavigate, Link); extract that wiring into a shared helper or mixin (e.g., createRouteHooks) which accepts the route identity (id, fullPath) and any Link/component references and returns the bound functions, then replace the inline implementations in RouteApi, Route, and RootRoute to delegate to that helper so all classes consume a single canonical implementation and avoid divergence.
JoviDeCroock
force-pushed
the
preact-integration
branch
from
f43310c to
edc0bfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/preact-router/src/link.tsx (1)
478-566: Event handlers recreated on every render; consideruseCallback.
handleClick,handleFocus,handleEnter,handleLeave, andhandleTouchStartare plain inline functions. Each render produces new references that propagate throughcomposeHandlersas newonClick/onFocus/etc. values, defeating memoization for any downstream component that checks prop reference equality.Wrapping each handler in
useCallbackwith an appropriate dependency array would stabilise the references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/preact-router/src/link.tsx` around lines 478 - 566, The event handlers (handleClick, handleFocus, handleEnter, handleLeave, and handleTouchStart) should be wrapped with React's useCallback to stabilize references; for each handler create a useCallback that lists only the values it reads as dependencies (e.g., handleClick depends on disabled, isCtrlEvent, setIsTransitioning, router, _options, replace, resetScroll, hashScrollIntoView, startTransition, viewTransition, ignoreBlocker; handleFocus depends on disabled, preload, doPreload; handleTouchStart should be a stable alias to the memoized handleFocus; handleEnter depends on disabled, preload, preloadDelay, doPreload, timeoutMap; handleLeave depends on disabled, preload, preloadDelay, timeoutMap). Ensure timeoutMap remains a stable reference (e.g., ref or module-level) or include it in dependencies so the callbacks remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/preact-router/src/link.tsx`:
- Around line 493-510: isTransitioning can get stuck and subscriptions leak:
ensure navigate failures and unmounts clear the transition and unsubscribe.
Update the click/navigation handler (where setIsTransitioning(true) is set,
router.subscribe('onResolved', ...) creates unsub, and router.navigate(...) is
called) to: wrap router.navigate(...) in try/catch/await (or handle its returned
promise) so failures/cancellations call setIsTransitioning(false); store unsub
and a flag (e.g., unsubRef/unsubbedRef) in a ref so a useEffect cleanup can call
unsub() and mark unsubbed to prevent setIsTransitioning on unmounted components;
inside the onResolved callback check the unsubbed flag before calling
setIsTransitioning(false) and always clear/unsubscribe after resolution or
error.
---
Duplicate comments:
In `@packages/preact-router/src/link.tsx`:
- Around line 767-772: The function-children callback signature
(LinkPropsChildren) expects both isActive and isTransitioning but only isActive
is passed; update the children invocation so it also passes a boolean
isTransitioning derived from cleanLinkProps (e.g., compute isTransitioning =
Boolean((cleanLinkProps)['data-transitioning'] ||
(cleanLinkProps)['data-is-transitioning'] ||
(cleanLinkProps)['data-transition'])) and pass it along with isActive when
calling rest.children so consumers receive both values.
---
Nitpick comments:
In `@packages/preact-router/src/link.tsx`:
- Around line 478-566: The event handlers (handleClick, handleFocus,
handleEnter, handleLeave, and handleTouchStart) should be wrapped with React's
useCallback to stabilize references; for each handler create a useCallback that
lists only the values it reads as dependencies (e.g., handleClick depends on
disabled, isCtrlEvent, setIsTransitioning, router, _options, replace,
resetScroll, hashScrollIntoView, startTransition, viewTransition, ignoreBlocker;
handleFocus depends on disabled, preload, doPreload; handleTouchStart should be
a stable alias to the memoized handleFocus; handleEnter depends on disabled,
preload, preloadDelay, doPreload, timeoutMap; handleLeave depends on disabled,
preload, preloadDelay, timeoutMap). Ensure timeoutMap remains a stable reference
(e.g., ref or module-level) or include it in dependencies so the callbacks
remain correct.
| await page.goto('/') | ||
| }) | ||
|
|
||
| test('shows suspense fallback and then resolved content', async ({ page }) => { |
There was a problem hiding this comment.
[nit]: test('shows suspense fallback and then resolves content', async ({ page }) => { .... }
| children: VNode | ||
| }) => { | ||
| try { | ||
| let html = await renderToStringAsync(children) |
There was a problem hiding this comment.
This is intentionally using our renderToStringAsync - currently preact doesn't have the best story for full document renders. Some examples of problems can be found in preactjs/preact#5029 and preactjs/preact#5028
That being said, RTSasync does a full render, including suspense boundaries
2 participants
Summary
Note
The streaming implementation isn't battle tested in preact, we however do export
renderToStringAsyncwhich resolves promises in-placeSupersedes #5782
This PR completes the initial Preact support work in TanStack Router
Suspenseimplementation frompreact-suspense.preact-suspenseandpreact-render-to-stringas runtime dependencies for@tanstack/preact-router.packages/preact-router/src/ssr:examples/basice2e/preact-router/basicSummary by CodeRabbit
New Features
Documentation
Tests
Chores