AppPlugins: Expose react-router to apps #33775

dprokop · 2021-05-06T11:08:32Z

An alternative solution to #33759

This simply makes the core app route not to e exact, and allows app plugins to use react-router directly. This gives the app plugin authors max flexiility to define their routes in the app.

Tricky thing is the base path for an app. I've changed the AppRootProps to expose basename property for navigation and routes creation downstream.

Note: react-router v6 will make the nested routes much easier. I did a quicl sketch of rr5 -> 6 update, but there is too big a risk to make this step right before v8 as there is plenty of changes to do. And RR6 is still in beta and learning from my previous experiences with RR they tend to change the APIs a lot before releasing stable.

See https://github.com/grafana/marketplace-app/pull/4 for a usage example.

TODO

Figure out how to build the nav model - do we need to build it on the backend?

domasx2 · 2021-05-10T06:34:04Z

Lovely!
Great if we can make core routes not exact as well, have use cases in unified alerting.

The only nit I see is that if plugins start defining routes with the basename included, it will break if we move to rr6 and basename becomes not needed anymore. Not sure what we can do about it though

dprokop · 2021-05-10T08:51:48Z

Great if we can make core routes not exact as well, have use cases in unified alerting.

@domasx2 not sure what and how would you accomplish anything with other routes not being exact. I mean - the App routes are rendered only under a/:pluginId segment which means you cannot use app route anywhere but under that segment. Wanna tell me something more about the use case you are describing?

domasx2 · 2021-05-10T09:06:43Z

Great if we can make core routes not exact as well, have use cases in unified alerting.

@domasx2 not sure what and how would you accomplish anything with other routes not being exact. I mean - the App routes are rendered only under a/:pluginId segment which means you cannot use app route anywhere but under that segment. Wanna tell me something more about the use case you are describing?

I think with this merged we'd be able to use exact={false} with any route in routes.tsx, not only related to plugins?

One example use case:
Let's say there are pages /alerting/silences, /alerting/silences/new and /alerting/silences/:id/edit. All of these share some common UI elements and load some common data. So I'd like to render only /alerting/silences in routes.tsx, with exact={false}. It would render some root silences component wich implements the common stuff and has further routing to nested pages. So that if you're going from /alerting/silences to /alerting/silences/new, the root silences component is not re-mounted, and common data is not re-fetched

dprokop · 2021-05-11T07:25:00Z

Ah, of course, this will be possible. I thought you were thinking of apps extending native routes.

torkelo · 2021-05-11T12:16:31Z

The only nit I see is that if plugins start defining routes with the basename included, it will break if we move to rr6 and basename becomes not needed anymore. Not sure what we can do about it though

@domasx2 what do you mean? we do not use this for any routes currently. what is changing in v6?

domasx2 · 2021-05-11T15:19:36Z

The only nit I see is that if plugins start defining routes with the basename included, it will break if we move to rr6 and basename becomes not needed anymore. Not sure what we can do about it though

@domasx2 what do you mean? we do not use this for any routes currently. what is changing in v6?

With this merged plugin devs will be able to use <Route /> with full path, eg /a/some-plugin/a-plugin-page. Then if later we upgrade to rr6 and want to put plugin under <Routes />, I assume that such existing routes would be interpreted as /a/some-plugin/a/some-plugin/a-plugin-page and stop working. So wondering if we can make a plan how to migrate them

plugins-bundled/internal/plugin-catalog-app/src/components/Card.tsx

dprokop · 2021-05-18T07:02:39Z

@ryantxu - have you seen https://github.com/grafana/marketplace-app/pull/4 ? It does exactly what you are trying to do in the last commit :)

ryantxu · 2021-05-18T23:53:08Z

packages/grafana-ui/src/index.ts

@@ -12,3 +12,4 @@ export * from './slate-plugins';

 // Exposes standard editors for registries of optionsUi config and panel options UI
 export { getStandardFieldConfigs, getStandardOptionEditors } from './utils/standardEditors';
+export { Route, useLocation, useParams } from 'react-router-dom';


Why export from here rather than just have people import directly? Is this to potentially help backwards compatibility? (if so, can you add a comment?)

no exports anymore, we are exposing RR to plugins and updating the plugin starters now

dprokop · 2021-05-19T08:49:19Z

This is a valid point. Would it be worth to try to upgrade to RR6 prior to adding this to the app API?

@mckn I've tried. It's plenty of work that's too risky to do during beta. And, the current relwase of RR6 is a prelease(beta0) so I would wait for a stable.

Regarding the support for RR6 -me and @torkelo talked about this and we think this approach should work OK with RR6 if we just change the root path passed to the app plugin from /a/:pluginId to /. Then all of the routes will become "nested" under the ap path.

…grafana-ui

domasx2 · 2021-05-19T16:33:06Z

Regarding the support for RR6 -me and @torkelo talked about this and we think this approach should work OK with RR6 if we just change the root path passed to the app plugin from /a/:pluginId to /. Then all of the routes will become "nested" under the ap path.

Ahh, perfect 👍

domasx2 · 2021-05-19T16:34:27Z

public/app/AppWrapper.tsx

@@ -59,7 +59,7 @@ export class AppWrapper extends React.Component<AppWrapperProps, AppWrapperState

    return (
      <Route
-        exact
+        exact={route.exact === undefined ? true : route.exact}


One of the great newish js features, fyi 😸

Suggested change

exact={route.exact === undefined ? true : route.exact}

exact={route.exact ?? true}

* Allow Route component usage in app plugins * i tried * fix catalog app * fix catalog app * remove catalog changes from this PR * remove extra files * feat(plugins): expose react-router to plugins rather than export via grafana-ui * Bring back query and pathname to AppRootPage and add deprecation notice Co-authored-by: Ryan McKinley <ryantxu@gmail.com> Co-authored-by: Jack Westbrook <jack.westbrook@gmail.com>

mckn · 2021-05-20T10:37:14Z

This is a valid point. Would it be worth to try to upgrade to RR6 prior to adding this to the app API?

@mckn I've tried. It's plenty of work that's too risky to do during beta. And, the current relwase of RR6 is a prelease(beta0) so I would wait for a stable.

Regarding the support for RR6 -me and @torkelo talked about this and we think this approach should work OK with RR6 if we just change the root path passed to the app plugin from /a/:pluginId to /. Then all of the routes will become "nested" under the ap path.

Ok ok, sounds good! Will this also prevent users from registering routes outside of the plugin path?

Allow Route component usage in app plugins

fde0754

dprokop added type/poc area/plugins/app App Plugins area/routing labels May 6, 2021

dprokop added this to the 8.0.0 milestone May 6, 2021

dprokop requested review from jackw, ryantxu, domasx2 and leventebalogh May 6, 2021 11:08

dprokop requested a review from a team May 6, 2021 11:08

dprokop self-assigned this May 6, 2021

dprokop requested review from mckn and removed request for a team May 6, 2021 11:08

grafanabot added the area/frontend label May 6, 2021

dprokop mentioned this pull request May 10, 2021

POC: use react-router in AppPlugins #33759

Closed

5 tasks

dprokop modified the milestones: 8.0.0-beta1, 8.0.0-beta2 May 13, 2021

ryantxu added 2 commits May 17, 2021 20:51

Merge remote-tracking branch 'origin/main' into routing-ng-app-plugins-2

a518d79

i tried

85dbeb0

ryantxu reviewed May 18, 2021

View reviewed changes

plugins-bundled/internal/plugin-catalog-app/src/components/Card.tsx Outdated Show resolved Hide resolved

dprokop requested review from oddlittlebird, achatterjee-grafana and osg-grafana and removed request for a team May 18, 2021 23:34

fix catalog app

9b3fac7

grafanabot added type/docs Flags the technical writing team for documentation support; auto adds to org-wide docs project area/backend labels May 18, 2021

ryantxu reviewed May 18, 2021

View reviewed changes

remove catalog changes from this PR

fac8eaa

dprokop requested a review from a team as a code owner May 18, 2021 23:54

ryantxu added 2 commits May 18, 2021 16:55

Merge remote-tracking branch 'origin/main' into routing-ng-app-plugins-2

9742365

remove extra files

6f2ec8a

ryantxu mentioned this pull request May 19, 2021

Plugin Admin App: make the catalog look like internal component #34341

Merged

jackw and others added 2 commits May 19, 2021 12:51

feat(plugins): expose react-router to plugins rather than export via …

02fae6c

…grafana-ui

Merge branch 'main' into routing-ng-app-plugins-2

58e5df9

jackw mentioned this pull request May 19, 2021

Chore: add react-router to package.json grafana/grafana-starter-panel#30

Closed

dprokop added 2 commits May 19, 2021 17:44

Merge branch 'main' into routing-ng-app-plugins-2

551e1b0

Bring back query and pathname to AppRootPage and add deprecation notice

e99c5a0

ryantxu approved these changes May 19, 2021

View reviewed changes

dprokop changed the title ~~POC: Allow Route component usage in app plugins~~ AppPlugins: Expose react-router to apps May 19, 2021

dprokop added add to changelog and removed type/poc labels May 19, 2021

domasx2 approved these changes May 19, 2021

View reviewed changes

dprokop merged commit 4cbffae into main May 19, 2021

dprokop deleted the routing-ng-app-plugins-2 branch May 19, 2021 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AppPlugins: Expose react-router to apps #33775

AppPlugins: Expose react-router to apps #33775

dprokop commented May 6, 2021 •

edited

Loading

domasx2 commented May 10, 2021

dprokop commented May 10, 2021

domasx2 commented May 10, 2021

dprokop commented May 11, 2021

torkelo commented May 11, 2021

domasx2 commented May 11, 2021

dprokop commented May 18, 2021 •

edited

Loading

ryantxu May 18, 2021

dprokop May 19, 2021

dprokop commented May 19, 2021 •

edited

Loading

domasx2 commented May 19, 2021

domasx2 May 19, 2021

mckn commented May 20, 2021

	exact={route.exact === undefined ? true : route.exact}
	exact={route.exact ?? true}

AppPlugins: Expose react-router to apps #33775

AppPlugins: Expose react-router to apps #33775

Conversation

dprokop commented May 6, 2021 • edited Loading

domasx2 commented May 10, 2021

dprokop commented May 10, 2021

domasx2 commented May 10, 2021

dprokop commented May 11, 2021

torkelo commented May 11, 2021

domasx2 commented May 11, 2021

dprokop commented May 18, 2021 • edited Loading

ryantxu May 18, 2021

Choose a reason for hiding this comment

dprokop May 19, 2021

Choose a reason for hiding this comment

dprokop commented May 19, 2021 • edited Loading

domasx2 commented May 19, 2021

domasx2 May 19, 2021

Choose a reason for hiding this comment

mckn commented May 20, 2021

dprokop commented May 6, 2021 •

edited

Loading

dprokop commented May 18, 2021 •

edited

Loading

dprokop commented May 19, 2021 •

edited

Loading