-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
feat(react-router): add useCanGoBack #3017
Conversation
View your CI Pipeline Execution ↗ for commit d471ca4.
☁️ Nx Cloud last updated this comment at |
packages/history/src/index.ts
Outdated
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
can we put this logic into assigKey()
?
packages/history/src/index.ts
Outdated
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, can we put this logic into assigKey()
?
packages/history/src/index.ts
Outdated
@@ -551,6 +568,7 @@ export function createMemoryHistory( | |||
entries[index] = path | |||
}, | |||
back: () => { | |||
console.log('back', 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.
remove
packages/history/src/index.ts
Outdated
@@ -51,17 +52,12 @@ export interface ParsedPath { | |||
|
|||
export interface HistoryState { | |||
key?: string | |||
index: number |
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 the key
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 an index
now could introduce a breaking change
return { router } | ||
} | ||
|
||
describe('history: History gives correct notifcations and state', () => { |
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.
describe('history: History gives correct notifcations and state', () => { | |
describe('history: History gives correct notifications and state', () => { |
}) | ||
|
||
const router = createRouter({ | ||
routeTree: rootRoute.addChildren([indexRoute, postsRoute]), |
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.
we should run the same test for all history types
subscriber({ location, action: { type: 'ROLLBACK' } }), | ||
) | ||
const handleIndexChange = (action: SubscriberHistoryAction) => { | ||
if (opts.notifyOnIndexChange ?? true) notify(action) |
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.
can you please always use { }
even for one liners?
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
so can we remove it?
…nd has index in state
Small question. Is this new Cause after updating my router version I started getting this error:
Should I manually assign it with some value (like |
no this is meant to be automatically set @ferretwithaberet we need to make it optional |
After updating I'm getting this error all over my code too.
|
fixes #2983