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

Update to react-router@6 #979

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

leventebalogh
Copy link
Contributor

@leventebalogh leventebalogh commented Nov 26, 2024

Fixes #608

Updates the scenes packages to use react-router v6.
This probably requires a major version bump, as it's going to be a breaking change for plugins that depend on scenes.

Why?

We are in the process of migrating Grafana core and also internal plugins to use react-router v6, however any plugin that is depending on scenes cannot easily do it (or at least it's a complicated task). Scenes still using react-router v5 is a blocker for plugins that would like to update.

Huge thanks to @jackw for the help of discovering and removing some of the circular dependences in the scenes package that were causing Jest to go mental.

Updating a plugin

Example plugin update PR →

  1. Bump @grafana/scenes
  2. Bump react-router-dom
-"react-router-dom": "^5.2.0",
+"react-router-dom": "^6.22.0",
  1. Remove @types/react-router-dom
-"@types/react-router-dom": "^5.2.0",
  1. <Route>: use relative wildcard routes
-<Route path="/a/myorg-example-app/home ... />
+<Route path="home/*" ... />
  1. <Route>: use the element prop
-<Route ... component={WithDrilldown} />
+<Route ... element={<WithDrilldown />} />
  1. <Switch>: replace with Routes
-import { Switch } from "react-router-dom";
+import { Routes } from "react-router-dom";  

-<Switch> ... </Switch>
+<Routes> ... </Routes>
  1. Add a relative wildcard routePath prop to each <SceneAppPage>
-new SceneAppPage({
-  url: "/a/myorg-example-app/home",
-  ...
- });
+new SceneAppPage({
+  url: "/a/myorg-example-app/home",
+  routePath: "home/*",
+  ...
+})

Testing

The following tests were done with a locally built Grafana and new plugin scaffolds.

  • Grafana with old scenes version & Plugin with new scenes version
  • Grafana with new scenes version & Plugin with old scenes version
  • Grafana with new scenes version & Plugin with new scenes version
📦 Published PR as canary version: 6.0.0--canary.979.12373078054.0

✨ Test out this PR locally via:

npm install @grafana/[email protected]
npm install @grafana/[email protected]
# or 
yarn add @grafana/[email protected]
yarn add @grafana/[email protected]

@leventebalogh leventebalogh self-assigned this Nov 26, 2024
@leventebalogh leventebalogh added the dependencies Update one or more dependencies version label Nov 26, 2024
@torkelo
Copy link
Member

torkelo commented Nov 27, 2024

Pushed an update to get SceneAppPage and most demos to work

Scene apps needs a big update

  • add routePath to all SceneAppPage definitions (with * wildcard if they have drilldowns) this needs to be relative not absolute
  • the url property on SceneAppPage needs to remain and be absolute as it's used to create breadcrumbs and tab links
  • routePath on all drilldowns needs to be relative and end in wildcard

<Route path={`${ROUTES.Demos}/*`} Component={DemoListPage} />
<Route path={`${ROUTES.GrafanaMonitoring}/*`} Component={GrafanaMonitoringApp} />
<Route path={`${ROUTES.ReactDemo}/*`} Component={ReactDemoPage} />
{/* <Redirect to={prefixRoute(ROUTES.Demos)} /> */}
Copy link
Member

Choose a reason for hiding this comment

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

Need to add a v6 redirect here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can just make DemoListPage a fallback page instead?

<Route path="*" Component={DemoListPage} />

@leventebalogh leventebalogh force-pushed the leventebalogh/update-react-router branch 4 times, most recently from e85e581 to 65f02c1 Compare December 16, 2024 13:03
@leventebalogh
Copy link
Contributor Author

leventebalogh commented Dec 16, 2024

I have spent sometime trying to green out all the test case, and although I think got somewhere, there are some weird things that might need someone with deeper knowledge of the scenes projects.

The main issue is with the tests under the scenes package: for some reason the inline jest.mock() calls don't seem to go through, while mocking via a __mocks__ directory is still working. Obviously it wouldn't make sense to transition all the tests to use a __mocks__ folder solution, we should instead find the root cause. (There was one issue we discovered with @jackw in the jest config: we also needed to introduce a "setupFiles" and move parts of the setupTestsAfterEnv.ts in there, although that didn't fix this problem.)

@grafana/dashboards-squad

@leventebalogh leventebalogh force-pushed the leventebalogh/update-react-router branch from 77516f0 to 8ee5f00 Compare December 17, 2024 09:50
@leventebalogh leventebalogh marked this pull request as ready for review December 17, 2024 12:19
@leventebalogh leventebalogh added release Create a release when this pr is merged major Increment the major version when merged labels Dec 17, 2024
@leventebalogh leventebalogh reopened this Dec 17, 2024
@oscarkilhed
Copy link
Contributor

I have spent sometime trying to green out all the test case, and although I think got somewhere, there are some weird things that might need someone with deeper knowledge of the scenes projects.

The main issue is with the tests under the scenes package: for some reason the inline jest.mock() calls don't seem to go through, while mocking via a __mocks__ directory is still working. Obviously it wouldn't make sense to transition all the tests to use a __mocks__ folder solution, we should instead find the root cause. (There was one issue we discovered with @jackw in the jest config: we also needed to introduce a "setupFiles" and move parts of the setupTestsAfterEnv.ts in there, although that didn't fix this problem.)

@grafana/dashboards-squad

@leventebalogh I see the tests are passing, did you find the issue?

Copy link
Contributor

@oscarkilhed oscarkilhed left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @leventebalogh

How does this work for apps built with scenes that run in Grafana? Will they continue to work as long as they are built with an older version of scenes, or will they break when Grafana and scenes runs on router@v6?

@leventebalogh
Copy link
Contributor Author

@leventebalogh I see the tests are passing, did you find the issue?

Yes, thanks @oscarkilhed, we managed to green them out with @jackw 👍

How does this work for apps built with scenes that run in Grafana? Will they continue to work as long as they are built with an older version of scenes, or will they break when Grafana and scenes runs on router@v6?

I am currently testing out these changes with an example plugin, and as soon as I am done with that I'll update the PR description with correct instructions on what steps plugins need to take for updating. (We will probably also have a migration page for this)

At the time being Grafana still supports plugins both using react-router@v5 or react-router@v6. This means, that plugins built with an earlier scenes version will still be supported (at least until core is updated to use react-router@v6 fully - then they will break). As far as I know scenes is bundled (and not externalised), so it shouldn't be a problem that core is depending on a different version (for now). I'll double check this though as well, and update the PR description!

This must be major version bump though, as any plugins updating to this scenes version will also need to update some things in their source code.

@leventebalogh
Copy link
Contributor Author

leventebalogh commented Dec 20, 2024

@torkelo @dprokop could you please have a look if you think it's legit? (I have updated the PR description with a guide for upgrading plugins.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update one or more dependencies version major Increment the major version when merged release Create a release when this pr is merged
Projects
Status: 💡 Ideation
Development

Successfully merging this pull request may close these issues.

Migrate react-router from v5 to v6
3 participants