From 1fa23f282cc974063ad27bca3bc1894cd627c749 Mon Sep 17 00:00:00 2001 From: "Jacques P. du Toit" Date: Sat, 8 Jun 2024 17:09:10 +0200 Subject: [PATCH 1/7] Prettier changes --- src/routing.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/routing.ts b/src/routing.ts index 6a9b09e0..91959bde 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -282,7 +282,7 @@ export function createRouterContext( integration: RouterIntegration, branches: () => Branch[], getContext?: () => any, - options: { base?: string; singleFlight?: boolean, transformUrl?: (url: string) => string } = {} + options: { base?: string; singleFlight?: boolean; transformUrl?: (url: string) => string } = {} ): RouterContext { const { signal: [source, setSource], @@ -494,9 +494,9 @@ export function createRouterContext( function initFromFlash() { const e = getRequestEvent(); - return (e && e.router && e.router.submission - ? [e.router.submission] - : []) as Array>; + return (e && e.router && e.router.submission ? [e.router.submission] : []) as Array< + Submission + >; } } @@ -504,7 +504,7 @@ export function createRouteContext( router: RouterContext, parent: RouteContext, outlet: () => JSX.Element, - match: () => RouteMatch, + match: () => RouteMatch ): RouteContext { const { base, location, params } = router; const { pattern, component, load } = match().route; From a22a432c8787eeb12c5d3eae72e74e5f1cb67308 Mon Sep 17 00:00:00 2001 From: "Jacques P. du Toit" Date: Sat, 8 Jun 2024 20:33:03 +0200 Subject: [PATCH 2/7] More reliable isRouting + other fixes --- src/routing.ts | 93 ++++++++++++++++++++++++--------------------- test/router.spec.ts | 43 +++++++++++++++++++-- 2 files changed, 89 insertions(+), 47 deletions(-) diff --git a/src/routing.ts b/src/routing.ts index 91959bde..49f67de5 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -1,4 +1,4 @@ -import { JSX, Accessor, runWithOwner } from "solid-js"; +import { JSX, Accessor, runWithOwner, batch } from "solid-js"; import { createComponent, createContext, @@ -301,13 +301,38 @@ export function createRouterContext( } const [isRouting, setIsRouting] = createSignal(false); - const start = async (callback: () => void) => { - setIsRouting(true); - try { - await startTransition(callback); - } finally { - setIsRouting(false); - } + let activeTransitions = 0; + + // Keep track of last intent and target, so that last call to transition wins + let lastTransitionIntent: Intent | undefined; + let lastTransitionTarget: LocationChange | undefined; + + // Transition the location to a new value + const transition = (newIntent: Intent, newTarget: LocationChange) => { + intent = lastTransitionIntent = newIntent; + lastTransitionTarget = newTarget; + + if (++activeTransitions === 1) setIsRouting(true); + + startTransition(() => + untrack(() => { + setReference(lastTransitionTarget!.value); + setState(lastTransitionTarget!.state); + resetErrorBoundaries(); + submissions[1]([]); + }) + ).finally(() => { + if (--activeTransitions !== 0) return; + + // Batch, in order for isRouting and final source update to happen together + batch(() => { + intent = undefined; + if (lastTransitionIntent === "navigate") { + navigateEnd(lastTransitionTarget!); + } + setIsRouting(false); + }); + }); }; const [reference, setReference] = createSignal(source().value); const [state, setState] = createSignal(source().state); @@ -341,22 +366,17 @@ export function createRouterContext( } }; + // Create a native transition, whenever the source changes from outside createRenderEffect(() => { - const { value, state } = source(); - // Untrack this whole block so `start` doesn't cause Solid's Listener to be preserved - untrack(() => { - start(() => { - intent = "native"; - if (value !== reference()) setReference(value); - setState(state); - resetErrorBoundaries(); - submissions[1]([]); - }).then(() => { - intent = undefined; - }); - }); + const newSource = source(); + if (newSource.value !== untrack(reference) || newSource.state !== untrack(state)) { + transition("native", newSource); + } }); + // Is this needed? + //createRenderEffect(() => transition("initial", untrack(source))); + return { base: baseRoute, location, @@ -418,21 +438,10 @@ export function createRouterContext( e && (e.response = { status: 302, headers: new Headers({ Location: resolvedTo }) }); setSource({ value: resolvedTo, replace, scroll, state: nextState }); } else if (beforeLeave.confirm(resolvedTo, options)) { - const len = referrers.push({ value: current, replace, scroll, state: state() }); - start(() => { - intent = "navigate"; - setReference(resolvedTo); - setState(nextState); - resetErrorBoundaries(); - submissions[1]([]); - }).then(() => { - if (referrers.length === len) { - intent = undefined; - navigateEnd({ - value: resolvedTo, - state: nextState - }); - } + referrers.push({ value: current, replace, scroll, state: state() }); + transition("navigate", { + value: resolvedTo, + state: nextState }); } } @@ -449,13 +458,11 @@ export function createRouterContext( function navigateEnd(next: LocationChange) { const first = referrers[0]; if (first) { - if (next.value !== first.value || next.state !== first.state) { - setSource({ - ...next, - replace: first.replace, - scroll: first.scroll - }); - } + setSource({ + ...next, + replace: first.replace, + scroll: first.scroll + }); referrers.length = 0; } } diff --git a/test/router.spec.ts b/test/router.spec.ts index ab12fe73..08a43c02 100644 --- a/test/router.spec.ts +++ b/test/router.spec.ts @@ -1,4 +1,4 @@ -import { createRoot, createSignal } from "solid-js"; +import { createComputed, createRoot, createSignal } from "solid-js"; import { createRouterContext } from "../src/routing.js"; import type { LocationChange } from "../src/types.js"; import { createAsyncRoot, createCounter, waitFor } from "./helpers.js"; @@ -338,8 +338,43 @@ describe("Router should", () => { }); // end of "have member `navigate`" describe("have member `isRouting` which should", () => { - test.skip("be true when the push or replace causes transition", () => { - throw new Error("Test not implemented"); - }); + test("be true when the push or replace causes transition", () => + createAsyncRoot(resolve => { + const signal = createSignal({ + value: "/" + }); + const { navigatorFactory, isRouting } = createRouterContext({ signal }, fakeBranches); + const navigate = navigatorFactory(); + + expect(isRouting()).toBe(false); + navigate("/target"); + expect(isRouting()).toBe(true); + waitFor(() => !isRouting()).then(resolve); + })); + + test("turn false, only after location has changed", () => + createAsyncRoot(resolve => { + const signal = createSignal({ + value: "/" + }); + const { navigatorFactory, isRouting } = createRouterContext({ signal }, fakeBranches); + const navigate = navigatorFactory(); + + navigate("/target"); + + // capture location immediately after `isRouting` turns false + let postRoutingValue: string | undefined; + createComputed(() => { + if (!isRouting() && !postRoutingValue) { + postRoutingValue = signal[0]().value; + } + }); + + return waitFor(() => !isRouting()) + .then(() => { + expect(postRoutingValue).toBe("/target"); + }) + .finally(resolve); + })); }); }); From 04756c3529a6167759e2fb522bb5c1abce9d86ad Mon Sep 17 00:00:00 2001 From: Brendan Allan Date: Wed, 12 Jun 2024 11:30:34 +0800 Subject: [PATCH 3/7] remove activeTransitions + some untracks --- src/routing.ts | 49 ++++++++++++++++++++++--------------------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/src/routing.ts b/src/routing.ts index 49f67de5..a4d9a80e 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -301,36 +301,37 @@ export function createRouterContext( } const [isRouting, setIsRouting] = createSignal(false); - let activeTransitions = 0; - // Keep track of last intent and target, so that last call to transition wins - let lastTransitionIntent: Intent | undefined; + // Keep track of last target, so that last call to transition wins let lastTransitionTarget: LocationChange | undefined; // Transition the location to a new value const transition = (newIntent: Intent, newTarget: LocationChange) => { - intent = lastTransitionIntent = newIntent; - lastTransitionTarget = newTarget; + if (newTarget.value === reference() && newTarget.state === state()) return; + + if (lastTransitionTarget === undefined) setIsRouting(true); - if (++activeTransitions === 1) setIsRouting(true); + intent = newIntent; + lastTransitionTarget = newTarget; - startTransition(() => - untrack(() => { - setReference(lastTransitionTarget!.value); - setState(lastTransitionTarget!.state); - resetErrorBoundaries(); - submissions[1]([]); - }) - ).finally(() => { - if (--activeTransitions !== 0) return; + startTransition(() =>{ + if(lastTransitionTarget !== newTarget) return; + setReference(lastTransitionTarget.value); + setState(lastTransitionTarget.state); + resetErrorBoundaries(); + submissions[1]([]); + }).finally(() => { // Batch, in order for isRouting and final source update to happen together batch(() => { + if (lastTransitionTarget !== newTarget) return; + intent = undefined; - if (lastTransitionIntent === "navigate") { - navigateEnd(lastTransitionTarget!); - } + if (newIntent === "navigate") navigateEnd(lastTransitionTarget); + setIsRouting(false); + + lastTransitionTarget = undefined; }); }); }; @@ -367,15 +368,9 @@ export function createRouterContext( }; // Create a native transition, whenever the source changes from outside - createRenderEffect(() => { - const newSource = source(); - if (newSource.value !== untrack(reference) || newSource.state !== untrack(state)) { - transition("native", newSource); - } - }); - - // Is this needed? - //createRenderEffect(() => transition("initial", untrack(source))); + createRenderEffect(on(source, (source) => { + transition("native", source); + })); return { base: baseRoute, From bab3d764defb4000ddd92fe0ea24dd3e102e9886 Mon Sep 17 00:00:00 2001 From: Brendan Allan Date: Wed, 12 Jun 2024 11:30:51 +0800 Subject: [PATCH 4/7] formatting --- src/routing.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/routing.ts b/src/routing.ts index a4d9a80e..f208715c 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -369,7 +369,7 @@ export function createRouterContext( // Create a native transition, whenever the source changes from outside createRenderEffect(on(source, (source) => { - transition("native", source); + transition("native", source); })); return { From 6eaa53e82fa0908402e33b5a8f3928b375cb9e13 Mon Sep 17 00:00:00 2001 From: "Jacques P. du Toit" Date: Wed, 12 Jun 2024 10:40:45 +0200 Subject: [PATCH 5/7] Defer first native transition --- src/routing.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/routing.ts b/src/routing.ts index f208715c..3bb82343 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -314,8 +314,8 @@ export function createRouterContext( intent = newIntent; lastTransitionTarget = newTarget; - startTransition(() =>{ - if(lastTransitionTarget !== newTarget) return; + startTransition(() => { + if (lastTransitionTarget !== newTarget) return; setReference(lastTransitionTarget.value); setState(lastTransitionTarget.state); @@ -367,10 +367,8 @@ export function createRouterContext( } }; - // Create a native transition, whenever the source changes from outside - createRenderEffect(on(source, (source) => { - transition("native", source); - })); + // Create a native transition, when source updates + createRenderEffect(on(source, source => transition("native", source), { defer: true })); return { base: baseRoute, From 1d445ebe0292921368977e280bf4d5ef93b26d59 Mon Sep 17 00:00:00 2001 From: "Jacques P. du Toit" Date: Wed, 12 Jun 2024 10:46:40 +0200 Subject: [PATCH 6/7] Only batch when needed --- src/data/cache.ts | 2 +- src/routing.ts | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/data/cache.ts b/src/data/cache.ts index ce12533d..600bd910 100644 --- a/src/data/cache.ts +++ b/src/data/cache.ts @@ -187,7 +187,7 @@ export function cache any>(fn: T, name: string): Cac return v; }; } - }) as CachedFunction; + }) as unknown as CachedFunction; cachedFn.keyFor = (...args: Parameters) => name + hashKey(args); cachedFn.key = name; return cachedFn; diff --git a/src/routing.ts b/src/routing.ts index 3bb82343..7a39d277 100644 --- a/src/routing.ts +++ b/src/routing.ts @@ -322,15 +322,14 @@ export function createRouterContext( resetErrorBoundaries(); submissions[1]([]); }).finally(() => { + if (lastTransitionTarget !== newTarget) return; + // Batch, in order for isRouting and final source update to happen together batch(() => { - if (lastTransitionTarget !== newTarget) return; - intent = undefined; - if (newIntent === "navigate") navigateEnd(lastTransitionTarget); + if (newIntent === "navigate") navigateEnd(lastTransitionTarget!); setIsRouting(false); - lastTransitionTarget = undefined; }); }); From 2f05f3733584c7ad82faeabe194908352ac34008 Mon Sep 17 00:00:00 2001 From: Ryan Carniato Date: Wed, 12 Jun 2024 12:45:30 +0100 Subject: [PATCH 7/7] Create new-walls-battle.md --- .changeset/new-walls-battle.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/new-walls-battle.md diff --git a/.changeset/new-walls-battle.md b/.changeset/new-walls-battle.md new file mode 100644 index 00000000..685fe3ef --- /dev/null +++ b/.changeset/new-walls-battle.md @@ -0,0 +1,5 @@ +--- +"@solidjs/router": patch +--- + +Make isRouting more reliable + other fixes