-
Notifications
You must be signed in to change notification settings - Fork 8.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
[Shared UX] Adoption of Shared UX Route component #150357
Changes from 20 commits
b507053
3e073dd
2ead4c6
0e26494
015f772
910a6ba
f9a1458
bf1aaa9
0f3875e
54b919a
7e79084
4ddfde6
b32743d
e9c3203
c99fba0
c8310d3
adfab30
02369d6
c2804e9
378c4a6
dc212be
5cf5bec
c05a676
4cda994
ef5632b
3f26d91
d2c7fdc
c5df904
6438c9c
050b774
d109c07
0ef4850
67c4b93
a2726af
3ece3f5
2c16bfa
766a997
c087f88
0d7a525
b6ee100
2852a5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,5 +18,6 @@ | |
"@kbn/share-plugin", | ||
"@kbn/utility-types", | ||
"@kbn/kibana-utils-plugin", | ||
"@kbn/shared-ux-router", | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,5 +21,6 @@ | |
"@kbn/kibana-react-plugin", | ||
"@kbn/aiops-utils", | ||
"@kbn/config-schema", | ||
"@kbn/shared-ux-router", | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,5 +31,6 @@ | |
"@kbn/i18n", | ||
"@kbn/core-mount-utils-browser-internal", | ||
"@kbn/config-schema", | ||
"@kbn/shared-ux-router", | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,5 +22,6 @@ | |
"@kbn/data-views-plugin", | ||
"@kbn/developer-examples-plugin", | ||
"@kbn/es-query", | ||
"@kbn/shared-ux-router", | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,8 @@ | |
*/ | ||
|
||
import React, { FunctionComponent, useMemo } from 'react'; | ||
import { Route, RouteComponentProps, Router, Switch } from 'react-router-dom'; | ||
import { RouteComponentProps, Router, Switch } from 'react-router-dom'; | ||
import { Route } from '@kbn/shared-ux-router'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Core's internal rendering and routing system is supposed to remain an implementation detail. Are we certain we want to use our custom Just so I understand, what will be the upsides? The main risk I see here is to increase the risk of cyclic dependencies in the future between internal/types core packages and shared ux packages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Route from @kbn/shared-ux-router is a wrapper around the react-router-dom but other imports haven't been migrated. This PR is the component that is now shared-ux-router #131805 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that's the component coming from #131805, then I think we don't need it for Core's internal router. But waiting for @majagrubic or @clintandrewhall's answer, in case I missed anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @pgayvallet if I'm following you here, you want There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, yeah I hadn't noticed the change to core here. I don't think we need it for the internal router, as long as downstream apps are using it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dc212be for changes thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Mostly to remove a risk of cyclic dependency between shared-ux component/packages and core 'internal' packages, yeah. Thanks for making the change @rshen91 |
||
import { History } from 'history'; | ||
import { EMPTY, Observable } from 'rxjs'; | ||
import useObservable from 'react-use/lib/useObservable'; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"id": "coverageFixtures", | ||
"owner": { "name": "Kibana Operations", "githubTeam": "kibana-operations" }, | ||
"version": "kibana", | ||
"server": false, | ||
"ui": true | ||
} | ||
|
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.
@lukeelmers @rshen I'm wondering if there's any value in re-exporting these types from Shared UX, rather than having the two packages repeated everywhere. It would give us the flexibility to change the types, if necessary, and reduce confusion or potential misuse.
If so, we could just run a refer replace, adding 'type' and switching 'react-router-dom' with the new lib...?
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.
No strong opinions from me on that, but I'd say you should only do it if you plan to add an eslint rule to catch anyone who tries to import from
react-router-dom
directly. And you'd probably want to have a plan for how to deal with new APIs that may get added in the future (would shared ux just export everything, or only certain things? etc)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.
6438c9c for eslint rule added thanks!