-
-
Notifications
You must be signed in to change notification settings - Fork 706
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
fix: ensured that history does not notify twice for certain actions and has index in state #3017
base: main
Are you sure you want to change the base?
Changes from 4 commits
264ade8
178fd7d
7a43517
abb6c55
c8a1cb0
36b91d6
01bf6a5
49df29b
73235f4
3407cf1
347e467
c5cf0a9
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 |
---|---|---|
|
@@ -8,7 +8,7 @@ export interface NavigateOptions { | |
|
||
type SubscriberHistoryAction = | ||
| { | ||
type: HistoryAction | 'ROLLBACK' | ||
type: HistoryAction | ||
} | ||
| { | ||
type: 'GO' | ||
|
@@ -30,6 +30,7 @@ export interface RouterHistory { | |
go: (index: number, navigateOpts?: NavigateOptions) => void | ||
back: (navigateOpts?: NavigateOptions) => void | ||
forward: (navigateOpts?: NavigateOptions) => void | ||
canGoBack: () => boolean | ||
createHref: (href: string) => string | ||
block: (blocker: NavigationBlocker) => () => void | ||
flush: () => void | ||
|
@@ -51,17 +52,12 @@ export interface ParsedPath { | |
|
||
export interface HistoryState { | ||
key?: string | ||
index: number | ||
} | ||
|
||
type ShouldAllowNavigation = any | ||
|
||
export type HistoryAction = | ||
| 'PUSH' | ||
| 'POP' | ||
| 'REPLACE' | ||
| 'FORWARD' | ||
| 'BACK' | ||
| 'GO' | ||
export type HistoryAction = 'PUSH' | 'REPLACE' | 'FORWARD' | 'BACK' | 'GO' | ||
|
||
export type BlockerFnArgs = { | ||
currentLocation: HistoryLocation | ||
|
@@ -105,9 +101,10 @@ export function createHistory(opts: { | |
back: (ignoreBlocker: boolean) => void | ||
forward: (ignoreBlocker: boolean) => void | ||
createHref: (path: string) => string | ||
canGoBack?: () => boolean | ||
flush?: () => void | ||
destroy?: () => void | ||
onBlocked?: (onUpdate: () => void) => void | ||
onBlocked?: () => void | ||
getBlockers?: () => Array<NavigationBlocker> | ||
setBlockers?: (blockers: Array<NavigationBlocker>) => void | ||
}): RouterHistory { | ||
|
@@ -119,11 +116,8 @@ export function createHistory(opts: { | |
subscribers.forEach((subscriber) => subscriber({ location, action })) | ||
} | ||
|
||
const _notifyRollback = () => { | ||
const _setLocation = () => { | ||
location = opts.getLocation() | ||
subscribers.forEach((subscriber) => | ||
subscriber({ location, action: { type: 'ROLLBACK' } }), | ||
) | ||
} | ||
|
||
const tryNavigation = async ({ | ||
|
@@ -149,7 +143,7 @@ export function createHistory(opts: { | |
action: actionInfo.type, | ||
}) | ||
if (isBlocked) { | ||
opts.onBlocked?.(_notifyRollback) | ||
opts.onBlocked?.() | ||
return | ||
} | ||
} | ||
|
@@ -174,7 +168,10 @@ export function createHistory(opts: { | |
} | ||
}, | ||
push: (path, state, navigateOpts) => { | ||
state = assignKey(state) | ||
// This is a fallback for when updating the router and there are history entries without index | ||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
const currentIndex = location.state.index ?? 0 | ||
state = assignKey({ ...state, index: currentIndex + 1 }) | ||
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. can we put this logic into |
||
tryNavigation({ | ||
task: () => { | ||
opts.pushState(path, state) | ||
|
@@ -187,7 +184,10 @@ export function createHistory(opts: { | |
}) | ||
}, | ||
replace: (path, state, navigateOpts) => { | ||
state = assignKey(state) | ||
// This is a fallback for when updating the router and there are history entries without index | ||
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition | ||
const currentIndex = location.state.index ?? 0 | ||
state = assignKey({ ...state, index: currentIndex }) | ||
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. same as above, can we put this logic into |
||
tryNavigation({ | ||
task: () => { | ||
opts.replaceState(path, state) | ||
|
@@ -203,7 +203,7 @@ export function createHistory(opts: { | |
tryNavigation({ | ||
task: () => { | ||
opts.go(index) | ||
notify({ type: 'GO', index }) | ||
_setLocation() | ||
}, | ||
navigateOpts, | ||
type: 'GO', | ||
|
@@ -213,7 +213,7 @@ export function createHistory(opts: { | |
tryNavigation({ | ||
task: () => { | ||
opts.back(navigateOpts?.ignoreBlocker ?? false) | ||
notify({ type: 'BACK' }) | ||
_setLocation() | ||
}, | ||
navigateOpts, | ||
type: 'BACK', | ||
|
@@ -223,12 +223,13 @@ export function createHistory(opts: { | |
tryNavigation({ | ||
task: () => { | ||
opts.forward(navigateOpts?.ignoreBlocker ?? false) | ||
notify({ type: 'FORWARD' }) | ||
_setLocation() | ||
}, | ||
navigateOpts, | ||
type: 'FORWARD', | ||
}) | ||
}, | ||
canGoBack: () => opts.canGoBack?.() ?? location.state.index !== 0, | ||
createHref: (str) => opts.createHref(str), | ||
block: (blocker) => { | ||
if (!opts.setBlockers) return () => {} | ||
|
@@ -301,6 +302,7 @@ export function createBrowserHistory(opts?: { | |
let currentLocation = parseLocation() | ||
let rollbackLocation: HistoryLocation | undefined | ||
|
||
let nextPopIsGo = false | ||
let ignoreNextPop = false | ||
let skipBlockerNextPop = false | ||
let ignoreNextBeforeUnload = false | ||
|
@@ -375,9 +377,10 @@ export function createBrowserHistory(opts?: { | |
} | ||
} | ||
|
||
const onPushPop = () => { | ||
// NOTE: this function can probably be removed | ||
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. so can we remove it? |
||
const onPushPop = (type: 'PUSH' | 'REPLACE') => { | ||
currentLocation = parseLocation() | ||
history.notify({ type: 'POP' }) | ||
history.notify({ type }) | ||
} | ||
|
||
const onPushPopEvent = async () => { | ||
|
@@ -386,30 +389,46 @@ export function createBrowserHistory(opts?: { | |
return | ||
} | ||
|
||
const nextLocation = parseLocation() | ||
const delta = nextLocation.state.index - currentLocation.state.index | ||
const isForward = delta === 1 | ||
const isBack = delta === -1 | ||
const isGo = (!isForward && !isBack) || nextPopIsGo | ||
nextPopIsGo = false | ||
|
||
const action = isGo ? 'GO' : isBack ? 'BACK' : 'FORWARD' | ||
const notify: SubscriberHistoryAction = isGo | ||
? { | ||
type: 'GO', | ||
index: delta, | ||
} | ||
: { | ||
type: isBack ? 'BACK' : 'FORWARD', | ||
} | ||
|
||
if (skipBlockerNextPop) { | ||
skipBlockerNextPop = false | ||
} else { | ||
const blockers = _getBlockers() | ||
if (typeof document !== 'undefined' && blockers.length) { | ||
for (const blocker of blockers) { | ||
const nextLocation = parseLocation() | ||
const isBlocked = await blocker.blockerFn({ | ||
currentLocation, | ||
nextLocation, | ||
action: 'POP', | ||
action, | ||
}) | ||
if (isBlocked) { | ||
ignoreNextPop = true | ||
win.history.go(1) | ||
history.notify({ type: 'POP' }) | ||
history.notify(notify) | ||
return | ||
} | ||
} | ||
} | ||
} | ||
|
||
currentLocation = parseLocation() | ||
history.notify({ type: 'POP' }) | ||
history.notify(notify) | ||
} | ||
|
||
const onBeforeUnload = (e: BeforeUnloadEvent) => { | ||
|
@@ -462,7 +481,10 @@ export function createBrowserHistory(opts?: { | |
ignoreNextBeforeUnload = true | ||
win.history.forward() | ||
}, | ||
go: (n) => win.history.go(n), | ||
go: (n) => { | ||
nextPopIsGo = true | ||
win.history.go(n) | ||
}, | ||
createHref: (href) => createHref(href), | ||
flush, | ||
destroy: () => { | ||
|
@@ -473,13 +495,11 @@ export function createBrowserHistory(opts?: { | |
}) | ||
win.removeEventListener(popStateEvent, onPushPopEvent) | ||
}, | ||
onBlocked: (onUpdate) => { | ||
onBlocked: () => { | ||
// If a navigation is blocked, we need to rollback the location | ||
// that we optimistically updated in memory. | ||
if (rollbackLocation && currentLocation !== rollbackLocation) { | ||
currentLocation = rollbackLocation | ||
// Notify subscribers | ||
onUpdate() | ||
} | ||
}, | ||
getBlockers: _getBlockers, | ||
|
@@ -491,13 +511,13 @@ export function createBrowserHistory(opts?: { | |
|
||
win.history.pushState = function (...args: Array<any>) { | ||
const res = originalPushState.apply(win.history, args as any) | ||
if (!history._ignoreSubscribers) onPushPop() | ||
if (!history._ignoreSubscribers) onPushPop('PUSH') | ||
return res | ||
} | ||
|
||
win.history.replaceState = function (...args: Array<any>) { | ||
const res = originalReplaceState.apply(win.history, args as any) | ||
if (!history._ignoreSubscribers) onPushPop() | ||
if (!history._ignoreSubscribers) onPushPop('REPLACE') | ||
return res | ||
} | ||
|
||
|
@@ -559,6 +579,7 @@ export function createMemoryHistory( | |
go: (n) => { | ||
index = Math.min(Math.max(index + n, 0), entries.length - 1) | ||
}, | ||
canGoBack: () => index !== 0, | ||
createHref: (path) => path, | ||
}) | ||
} | ||
|
@@ -587,7 +608,7 @@ export function parseHref( | |
searchIndex > -1 | ||
? href.slice(searchIndex, hashIndex === -1 ? undefined : hashIndex) | ||
: '', | ||
state: state || {}, | ||
state: state || { index: 0 }, | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
import { useSyncExternalStore } from 'react' | ||
import { useRouter } from './useRouter' | ||
import type { RouterHistory } from '@tanstack/history' | ||
|
||
export function useCanGoBack() { | ||
const router = useRouter() | ||
const history: RouterHistory = router.history | ||
|
||
return useSyncExternalStore(history.subscribe, history.canGoBack) | ||
} |
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.
shouldn't this be
index?: number
as the index is not set for the initial entry?also, i think we should use a more ... obscure name to not collide with user data
e.g.
__TSR_index
?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.
It's always available after we run through
parseHref
, this is because, if the route has no state (should be valid only for initial route), we default it to{ index: 0 }
. About the naming, sure, but by that logic, shouldn't thekey
also be renamed?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.
ideally, yes. but the
key
has been there since the beginning, but adding anindex
now could introduce a breaking change