-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Add support for React router v7 #10440
Conversation
@@ -1,6 +1,6 @@ | |||
import * as React from 'react'; | |||
import Button from '@mui/material/Button'; | |||
import { Link } from 'react-router-dom'; | |||
import { Link } from 'react-router'; |
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 used to be only available from react-router-dom
so it "proves" we use v7
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 a non breaking change way to migrate to react-router v7 is always use react-router-dom package inside react-admin. It is a re-export of react-router https://reactrouter.com/start/changelog#package-restructuring
Then people using react-router v6 won't be affected by such things like import { Link } from 'react-router(v6)'; that does not exist
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 I see it is for demo project, not package itself. I misunderstood the previous message.
This reverts commit 1d4d83f.
41ba730
to
ef71b58
Compare
"react-router": "^7.0.0", | ||
"react-router-dom": "^7.0.0", |
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.
Note: only the e-commerce demo will use react-router v7.
I can't do the same for the tutorial demo, as we are forced to have duplicate versions of react-router in our monorepo, and vite will tree-shake the v7 if we don't use it in the actual code of the demo.
|
Should be fixed, back to RFR! |
@slax57 and @djhi - just an FYI, I just had dependabot try to bump my project to react-router-dom v7, and I started getting a routing error.
My use case is that I mount react-admin under the route "/manage" for my admin dashboard. Example in App.tsx:
Example CustomAdminRoutes:
I tried implementing the solution from their docs on relative splat paths: (https://reactrouter.com/en/v6/upgrading/future#v7_relativesplatpath) to the following in my App.tsx:
But I'm still getting the same error. Just perusing this PR it doesn't appear there's many updates aside from just bumping version references. I can see in your tests that you are testing use of baseName here - react-admin/packages/react-admin/src/Admin.stories.tsx Lines 54 to 61 in 4bdf7b5
createMemoryRouter instead of createBrowserRouter - unsure if this is the difference in why I'm getting an error or not. Would love any insight you guys have as this seems to be isolated to just my react-admin dashboard paths. Thanks in advance for any insight you can provide!
|
Thanks @streetlogics for this early feedback! FYI, with this PR, in our monorepo, all projects (including the storybook) except the e-commerce demo (under That being said, I reproduced your use-case in the demo (which, as said above, uses react-router v7) and it worked with no issue. I did not run into the error you mentioned. Can you try to see if you reproduce your issue in the demo? |
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.
💪
@slax57 - I was able to replicate the issue with the demo project by doing the following:
I did some digging around in my own project in the dependabot updates branch, and I added this to my base package.json:
When I re-ran yarn install and launched my environment, the warning went away, and everything rendered as expected. I checked my yarn.lock file and it had the following updates: I tried doing a similar thing by manually adding the resolutions both in the root package.json as well as in the demo project on this branch in my fork, but was not able to see the same lockfile change and still saw the same warning in the browser |
@streetlogics Thanks for the additional information. I'm not sure if this is the same issue as the one described in your initial post, but the warning you get after updating the demo the way you describe is to be expected. if, instead, you visit http://localhost:8000/#/manage, then the warning should be gone. In any case, I'm glad you managed to solve the issue on your end. If ever you encounter this warning again during your tests, feel free to open a new issue about it. Cheers, |
Problem
react-router v7 is out
Solution
Follow #10437 and #10439
Add support for React router v7
Find a way to avoid resolutions in projects out of our monorepo
react-admin packages will still use v6 in their devDependencies to ensure we don't use v7 only features inadvertently.
How To Test
Works with v6
npm pack
in thera-core
ra-ui-materialui
react-admin
ra-data-json-server
ra-i18n-polyglot
ra-language-english
package directoriesnpm init react-admin react-admin-router
npm add react-router@6 react-router-dom@6
ra-core
ra-ui-materialui
react-admin
ra-data-json-server
ra-i18n-polyglot
ra-language-english
deps by specifying the archive file you built in 1Works with v7
^7.0.0
npm install
Additional Checks
master
for a bugfix, ornext
for a feature