Skip to content
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

Merged
merged 6 commits into from
Jan 15, 2025
Merged

Add support for React router v7 #10440

merged 6 commits into from
Jan 15, 2025

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Jan 9, 2025

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

  1. build the RA packages locally from this branch and npm pack in the ra-core ra-ui-materialui react-admin ra-data-json-server ra-i18n-polyglot ra-language-english package directories
  2. create a new react-admin app: npm init react-admin react-admin-router
  3. add the react-router deps: npm add react-router@6 react-router-dom@6
  4. add 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 1
  5. Run the app & check your lock file

Works with v7

  • change the previous app react-router dependencies to ^7.0.0
  • npm install
  • Run the app & check your lock file

Additional Checks

  • The PR targets master for a bugfix, or next for a feature

@@ -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';
Copy link
Collaborator Author

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

Copy link
Contributor

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

Copy link
Contributor

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.

Comment on lines +32 to +33
"react-router": "^7.0.0",
"react-router-dom": "^7.0.0",
Copy link
Contributor

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.

@slax57 slax57 added RFR Ready For Review and removed WIP Work In Progress labels Jan 14, 2025
@slax57
Copy link
Contributor

slax57 commented Jan 14, 2025

react-admin stories are failing. Back to WIP.

@slax57 slax57 added WIP Work In Progress and removed RFR Ready For Review labels Jan 14, 2025
@slax57
Copy link
Contributor

slax57 commented Jan 14, 2025

Should be fixed, back to RFR!

@slax57 slax57 added RFR Ready For Review and removed WIP Work In Progress labels Jan 14, 2025
@streetlogics
Copy link

streetlogics commented Jan 15, 2025

@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.
Error:

router.js:247 <Router basename="/manage"> is not able to match the URL "/" because it does not start with the basename, so the <Router> won't render anything.

My use case is that I mount react-admin under the route "/manage" for my admin dashboard.

Example in App.tsx:

function App() {
  const router = createBrowserRouter([
    {
      path: '/manage/*',
      element: <CustomAdminRoutes />,
    },
  ]);
  return (
    <RouterProvider router={router} />
  )
}

Example CustomAdminRoutes:

const CustomAdminRoutes = () => {
  return (
    <Admin
      basename="/manage"
    >
      <Resource name="someResource" />
    </Admin>
}

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:

function App() {
  const router = createBrowserRouter([
    {
      path: '/manage',
      children: [{ path: '*', element: <CustomAdminRoutes /> }],
    },
  ]);
  return (
    <RouterProvider router={router} />
  )
}

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 -

<Route
path="/admin/*"
element={
<Admin dataProvider={testDataProvider()} basename="/admin">
<Resource name="posts" list={PostList} />
<Resource name="comments" list={CommentList} />
</Admin>
}
- looks like tracing that back up that uses 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!

@slax57
Copy link
Contributor

slax57 commented Jan 15, 2025

Thanks @streetlogics for this early feedback!

FYI, with this PR, in our monorepo, all projects (including the storybook) except the e-commerce demo (under examples/demo) are still using react-router v6.
Testing the storybook using react-router v7 has to be done manually.

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?

Copy link
Collaborator Author

@djhi djhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💪

@djhi djhi merged commit e26362c into next Jan 15, 2025
15 checks passed
@djhi djhi deleted the react-router-v7 branch January 15, 2025 09:14
@djhi djhi added this to the 5.5.0 milestone Jan 15, 2025
@streetlogics
Copy link

streetlogics commented Jan 16, 2025

@slax57 - I was able to replicate the issue with the demo project by doing the following:

  1. Checkout next branch
  2. run yarn install from root
  3. Open examples/demo/src/App.tsx and updated the component on line 50 with: basename="/manage"
  4. Run yarn run-demo
  5. Visit localhost:8000/ or localhost:8000/manage
  6. Observe console warning saying <Router basename="/manage"> is not able to match the URL "/" because it does not start with the basename, so the <Router> won't render anything.

I did some digging around in my own project in the dependabot updates branch, and I added this to my base package.json:

  "resolutions": {
    "react-router": "^7.1.1",
    "react-router-dom": "^7.1.1"
  },

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:
image

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 ¯\_(ツ)_/¯. At this point I have my project working, and suspect that once the official dependency upgrade to 7.1.1 for react-router / react-router-dom makes it into the project that everything will just "Work As Expected ™️"

@slax57
Copy link
Contributor

slax57 commented Jan 16, 2025

@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.
Indeed, the demo uses the default react-admin router, i.e. a HashRouter (not a BrowserRouter).
Hence, visiting either localhost:8000/ or localhost:8000/manage resolves to the same: from the react-router perspective, the location is still /, which matches no route declared in the App.
So the warning is normal.

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,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants