-
Notifications
You must be signed in to change notification settings - Fork 13.4k
feat(react-router): upgrade to react router 6 #30831
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
base: major-9.0
Are you sure you want to change the base?
Conversation
Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
…ter 6 Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
…ct router 6 Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
… router 6 Co-authored-by: Sean Perkins <13732623+sean-perkins@users.noreply.github.com>
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.
Would this be beneficial to add to main now?
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 mean, it's a quality of life upgrade for the script. It behaves the same, it just makes it significantly more clean and uses the npm built-in commands instead of rolling our own code here. Like, it's fine to put it in main, but it's easier to just let it go to main through the v9 update.
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 for personal usage?
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.
Yes, I also think it's good for everyone in the same way sync is. It does a lot of the annoying work of building and starting the server and killing the old one for you, which makes iteration much nicer. I 100% intend to commit this to main.
I'll probably create similar scripts for the other packages as we go.
| * @param options The options for computing the parent path. | ||
| * @returns The computed parent path result. | ||
| */ | ||
| export const computeParentPath = (options: ComputeParentPathOptions): ParentPathResult => { |
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 wonder if we can split some of the logic of computeParentPath to smaller functions so it's easier to digest the if statements.
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.
Done! 6f184e5
| useEffect(() => { | ||
| const activeView = viewStack.current.findViewItemByRouteInfo(routeInfo, undefined, true); | ||
| const matchedParams = activeView?.routeData.match?.params as RouteParams | undefined; | ||
|
|
||
| if (matchedParams) { | ||
| const paramsCopy = filterUndefinedParams({ ...matchedParams }); | ||
| if (areParamsEqual(routeInfo.params as RouteParams | undefined, paramsCopy)) { | ||
| return; | ||
| } | ||
| } else { | ||
| this.handleNavigate(pathname, 'push', 'none', undefined, routeOptions, tab); | ||
|
|
||
| const updatedRouteInfo: RouteInfo = { | ||
| ...routeInfo, | ||
| params: paramsCopy, | ||
| }; | ||
| locationHistory.current.update(updatedRouteInfo); | ||
| setRouteInfo(updatedRouteInfo); | ||
| } | ||
| } | ||
| }, [routeInfo]); |
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.
Let's add a comment of what this is doing.
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.
| <IonHeader> | ||
| <IonToolbar> | ||
| <IonTitle>Main</IonTitle> | ||
| <IonTitle>Test App - React Router v{majorVersion}</IonTitle> |
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.
Let's make this change on main.
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 doesn't seem necessary to port this to main when this is the first time we're updating the version.
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 agree with Brandy, but also I don't think this change was necessary. This was very useful when I was side-by-side comparing the RR5 and RR6 test apps to see what functionality was working before vs working now, but we can never actually have multiple RR versions in the same branch, so it doesn't really make sense to keep around now. I'm just going to revert this back to saying "Main"
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 it could be useful if we ever support multiple versions at once, but yeah probably not now.
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.
| // cy.get('ion-menu.show-menu').should('exist'); | ||
| // cy.wait(1000) |
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.
Let's get rid of the commented out code.
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.
| this.ionLifeCycleContext = context; | ||
| return ( | ||
| <StackManager routeInfo={routeInfo}> | ||
| <StackManager routeInfo={routeInfo} id={id}> |
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.
Nitpick
| <StackManager routeInfo={routeInfo} id={id}> | |
| <StackManager id={id} routeInfo={routeInfo}> |
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.
| setRef={(val: HTMLIonRouterOutletElement) => (this.ionRouterOutlet = val)} | ||
| id={id} |
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.
Nitpick
| setRef={(val: HTMLIonRouterOutletElement) => (this.ionRouterOutlet = val)} | |
| id={id} | |
| id={id} | |
| setRef={(val: HTMLIonRouterOutletElement) => (this.ionRouterOutlet = val)} |
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.
|
|
||
| # Install Dependencies | ||
| npm install *.tgz --no-save | ||
| npm install *.tgz --no-save --legacy-peer-deps |
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.
Let's add a note of why legacy-peer-deps is needed and a TODO to remove it when possible.
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.
…o sk/react-router-6
…gation (#30854) Issue number: resolves internal --------- <!-- Please do not submit updates to dependencies unless it fixes an issue. --> <!-- Please try to limit your pull request to one type (bugfix, feature, etc). Submit multiple pull requests if needed. --> ## What is the current behavior? When switching between tabs using the tab bar in a React Router 6 application, clicking IonBackButton incorrectly navigates back to the previous tab. Each tab should have its own isolated navigation history stack, and the back button should only navigate within the current tab's history. ## What is the new behavior? Tab navigation history is now properly isolated. When switching tabs via the tab bar: - The back button only navigates within the current tab's history - Pressing back on a tab root (with no navigation history) does nothing instead of navigating to the previous tab or default route - Within-tab navigation continues to work correctly (e.g., navigating to a details page and back) - Tab history is preserved when switching away and returning to a tab ## Does this introduce a breaking change? - [ ] Yes - [X] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information <!-- Any other information that is important to this PR such as screenshots of how the component looks before and after the change. --> Current dev build: ``` 8.7.12-dev.11765377112.16762e5b ```
8104d09 to
6f3bba7
Compare
6f3bba7 to
0c82ee2
Compare
0c82ee2 to
6a61ecf
Compare
brandyscarney
left a comment
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.
Looking really good! 🚀 My comments are mostly questions & potential issues with either the test app or the router code.
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.
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.
After merging all of my other RR changes into this branch (07104c2), I seem unable to replicate this. I believe this was fixed indirectly in the change that fixed changes during navigation, because it also fixed an issue with certain navigation directions. If you're able to, please pull and try this one again
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.
Multiple Tabs
- Run the test app locally
- Navigate to
Multiple Tabs - Open the Menu and go to
Tab 2 - Click
Page D - 🐛 See a flicker on the menu toggle button (note: this sometimes happens going from
Page AtoPage Balso):
route-menu-bug.mov
- Refresh the page
- 🐛 See nothing loaded in the view
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.
Nested Outlet
Note
It is easier to see the transition issues in ios mode, so append ?ionic:mode=ios to the URL to switch for this test.
- Run the test app locally
- Navigate to
Nested Outlet - Open the console
- Click on
Go to second page - 🐛 See that there is no transition here
- Click on
Back with direction "back" - Click on
Go to second pageagain - 🐛 Repeat until you see a log when clicking
Go to second pagewithout navigation, see video below
nested-outlet-bug.mov
| <IonHeader> | ||
| <IonToolbar> | ||
| <IonTitle>Main</IonTitle> | ||
| <IonTitle>Test App - React Router v{majorVersion}</IonTitle> |
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 doesn't seem necessary to port this to main when this is the first time we're updating the version.
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.
What was the reason for this update?
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.
This is a massive QoL improvement for this file, we go from rolling our own sync to using npm's integrated sync. This should make maintaining this file a lot easier and it's generally less fragile.
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.
Okay I am just worried about it getting out of sync somehow. 😂 We should make a task to update the others the same way if it works the same.
packages/react-router/src/ReactRouter/utils/computeParentPath.ts
Outdated
Show resolved
Hide resolved
…RouterOutlet (#30844) Issue number: resolves [an issue from a comment](#24177 (comment)) --------- ## What is the current behavior? <!-- Please describe the current behavior that you are modifying. --> Currently, Ionic's RR6 doesn't support relative routes in the same way RR6 does. Routes that do not start with a `/`do not work in the Ionic RR6 implementation in some cases. ## What is the new behavior? With this change, we properly support these route styles and more closely align with normal RR6 route support. ## Does this introduce a breaking change? - [ ] Yes - [X] No <!-- If this introduces a breaking change: 1. Describe the impact and migration path for existing applications below. 2. Update the BREAKING.md file with the breaking change. 3. Add "BREAKING CHANGE: [...]" to the commit description when merging. See https://github.com/ionic-team/ionic-framework/blob/main/docs/CONTRIBUTING.md#footer for more information. --> ## Other information Current dev build: ``` 8.7.12-dev.11765307927.1f491e92 ``` This PR will be merged into the RR6 branch, which will be squash+merged into the major 9 branch. This will prevent major 9 from having commits in the change log on release that reference fixing things that are only available in major 9.
…p, and a bit of ordering
2929474 to
ab3dde2
Compare

Issue number: resolves #24177
What is the current behavior?
Currently, Ionic Framework React Router only supports React Router 5. This has many issues and unsupported/broken features.
What is the new behavior?
With this change, Ionic Framework will support React Router 6 while still supporting transitions in the same way a native app does.
Most of what caused this change to take a long time is that React Router 5 and React Router 6 have fundamental differences in how they handle components once they're no longer part of the view. In this change, we move away from relying on React Router directly so much and have our own implementation for deciding how views get dealt with during navigation and when they're cleaned up, allowing for us to still transition between them like we need to while still using React Router as much as we possibly can.
This change will also lay the foundation for the migration to React Router 7, which will ideally be easier since most of the hard work has been dealt with here.
Does this introduce a breaking change?
Other information
Current dev build: