-
Notifications
You must be signed in to change notification settings - Fork 30.3k
Finish deleting Mutable from router implementation #88046
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
Conversation
28caced to
f2c577c
Compare
f2c577c to
9fb28a7
Compare
Tests Passed |
packages/next/src/client/components/segment-cache/navigation.ts
Outdated
Show resolved
Hide resolved
1b019e3 to
000079e
Compare
000079e to
fe11125
Compare
7369e09 to
4bfd70f
Compare
| const currentRenderedSearch = state.renderedSearch | ||
| const currentFlightRouterState = state.tree | ||
| const shouldScroll = true | ||
| const shouldScroll = false |
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.
was this an intentional behavior change?
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.
The net behavior is the same, it probably should have been false before but was working anyway for other reasons. There are lots of existing tests for this so I'm pretty confident it's safe.
| export const RedirectType = { | ||
| Push: 'push', | ||
| Replace: 'replace', | ||
| } |
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.
I think we should keep the casing from before this refactor, eg RedirectType.push rather than RedirectType.Push so we don't break apps that were depending on this
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.
oh oops that was unintentional, good catch
| shouldScroll, | ||
| navigateType | ||
| ).catch(() => { | ||
| // If the navigation fails, return the current 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.
wonder if it's worth logging something during dev when this happens
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.
Probably should though to be clear that's the current behavior, too
| url.search === oldUrl.search && | ||
| url.hash !== oldUrl.hash | ||
|
|
||
| // During a hash-only change, setting scrollableSegmeths to an empty |
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.
| // During a hash-only change, setting scrollableSegmeths to an empty | |
| // During a hash-only change, setting scrollableSegments to an empty |
4bfd70f to
6b1f27d
Compare
The Mutable type was used by an earlier implementation of App Router when the reducer used to run during React's render phase. It was used to prevent certain operations from running multiple times if an update was reapplied by React. Now that router "reducer" actions run outside React's render phase, we no longer need this indirection — we can compute the next state object directly. Most of the Mutable-related logic was deleted in previous PRs; this finishes the migration by inlining handleMutable into its callers.
6b1f27d to
09aa63c
Compare
Based on:
The Mutable type was used by an earlier implementation of App Router when the reducer used to run during React's render phase. It was used to prevent certain operations from running multiple times if an update was reapplied by React.
Now that router "reducer" actions run outside React's render phase, we no longer need this indirection — we can compute the next state object directly.
Most of the Mutable-related logic was deleted in previous PRs; this finishes the migration by inlining handleMutable into its callers.