Update react-router-dom to v5 #46885
Conversation
|
@spalger I wasn't sure if I should tag teams which use |
💔 Build Failed |
💔 Build Failed |
|
I'm not sure how to deal with these CI failures. Here are some notes on each. I've cc'd some people from the sections which are failing as a way to gain insight into how/why they're failing. This PR is supposed to be non-breaking so I'm looking for some clues into what might have gone wrong (e.g. updated a related lib to a new version?). Please don't think you need to fix the issues here. I'd just appreciate any thoughts you have on what might be going on. I'd love to get us to React Router 5 and would appreciate any help you can give; whatever that looks like. Type CheckErrors seem related to these tickets about I got rid of the errors using this diff, but I'm not sure if it's correct diff to fix type check errors https://github.com/microsoft/TypeScript/issues/33490diff --git a/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx b/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx
index 8d4c7703df..e266c47a60 100644
--- a/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx
+++ b/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx
@@ -14,8 +14,8 @@ interface LayoutProps {
modalClosePath?: string;
}
-export const NoDataLayout: React.SFC<LayoutProps> = withRouter<any>(
- ({ actionSection, title, modalClosePath, children, history }) => {
+export const NoDataLayout = withRouter<any, React.FunctionComponent<LayoutProps>>(
+ ({ actionSection, title, modalClosePath, children }) => {
return (
<EuiFlexGroup justifyContent="spaceAround">
<EuiFlexItem grow={false}>
@@ -31,4 +31,4 @@ export const NoDataLayout: React.SFC<LayoutProps> = withRouter<any>(
</EuiFlexGroup>
);
}
-) as any;
+);
diff --git a/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx b/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx
index 30d12c9ce1..44c6822747 100644
--- a/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx
+++ b/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx
@@ -8,6 +8,14 @@ import React from 'react';
import { EuiLink } from '@elastic/eui';
import { Link, withRouter } from 'react-router-dom';
+interface ConnectedLinkComponentProps {
+ location: any;
+ path: string;
+ disabled: boolean;
+ query: any;
+ [key: string]: any;
+}
+
export function ConnectedLinkComponent({
location,
path,
@@ -15,13 +23,7 @@ export function ConnectedLinkComponent({
disabled,
children,
...props
-}: {
- location: any;
- path: string;
- disabled: boolean;
- query: any;
- [key: string]: any;
-}) {
+}: ConnectedLinkComponentProps) {
if (disabled) {
return <EuiLink aria-disabled="true" {...props} />;
}
@@ -38,4 +40,6 @@ export function ConnectedLinkComponent({
);
}
-export const ConnectedLink = withRouter<any>(ConnectedLinkComponent);
+export const ConnectedLink = withRouter<any, React.FC<ConnectedLinkComponentProps>>(
+ ConnectedLinkComponent
+);
diff --git a/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx b/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx
index 0802b4d8ea..ad5fd767c0 100644
--- a/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx
+++ b/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx
@@ -86,7 +86,7 @@ export class WithURLStateComponent<URLState extends object> extends React.Compon
});
};
}
-export const WithURLState = withRouter<any>(WithURLStateComponent);
+export const WithURLState = withRouter<any, any>(WithURLStateComponent);
export function withUrlState<OP>(
UnwrappedComponent: React.ComponentType<OP & URLStateProps>/cc @mattapperson CI Group 1,4The failures for Group 1 and Group 4 are all This is supposed to be a non-breaking change, so these make me uncomfortable. However, the Group 4 failures seem confined to /cc @cjcenizal @sebelga since I saw your names in the history when looking at these lines CI Group 2The Group 2 failures have some /cc @mw-ding b/c I saw your name in the tests |
|
@elasticmachine merge upstream |
|
@jfsiii Sorry, can you link to the CI failure you are seeing for types? I can't seem to find it in the CI logs of the last fail |
|
@mattapperson This is from the first failed run, but the errors are the same https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/1443/execution/node/45/log/ I accidentally committed the diff I posted so the last run passed without type errors, but I've since reverted that and expect the third run to fail like the first. Like I said, I think the issue is with the |
|
@sebelga In #36871 you mentioned this:
Were the things that broke the same things that @jfsiii is reporting? If so, then it sounds like we just need to migrate these three apps off of the context-based method of accessing the router. If this is the case, could you provide @jfsiii with steps for doing that, so he can make those changes in this PR? |
💔 Build Failed |
|
@jfsiii I think the types changed in ReactRouter from the docs it looks like it needs to be adjusted to: interface ComponentProps extends RouteComponentProps { and then type the arguments as it is no longer inferred in this package through to the component |
💔 Build Failed |
|
@mattapperson Not sure if this is what you were thinking but it also clears the type warnings: diff --git a/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx b/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx
index 8d4c7703df..e316f6d146 100644
--- a/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx
+++ b/x-pack/legacy/plugins/beats_management/public/components/layouts/no_data.tsx
@@ -5,17 +5,17 @@
*/
import { EuiEmptyPrompt, EuiFlexGroup, EuiFlexItem, EuiPageContent } from '@elastic/eui';
-import React from 'react';
-import { withRouter } from 'react-router-dom';
+import React, { ComponentProps } from 'react';
+import { withRouter, RouteComponentProps } from 'react-router-dom';
-interface LayoutProps {
+interface LayoutProps extends ComponentProps<any>, RouteComponentProps {
title: string | React.ReactNode;
actionSection?: React.ReactNode;
modalClosePath?: string;
}
-export const NoDataLayout: React.SFC<LayoutProps> = withRouter<any>(
- ({ actionSection, title, modalClosePath, children, history }) => {
+export const NoDataLayout = withRouter(
+ ({ actionSection, title, modalClosePath, children, history }: LayoutProps) => {
return (
<EuiFlexGroup justifyContent="spaceAround">
<EuiFlexItem grow={false}>
@@ -31,4 +31,4 @@ export const NoDataLayout: React.SFC<LayoutProps> = withRouter<any>(
</EuiFlexGroup>
);
}
-) as any;
+);
diff --git a/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx b/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx
index 30d12c9ce1..501ddaf410 100644
--- a/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx
+++ b/x-pack/legacy/plugins/beats_management/public/components/navigation/connected_link.tsx
@@ -3,10 +3,18 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
-import React from 'react';
+import React, { ComponentProps } from 'react';
import { EuiLink } from '@elastic/eui';
-import { Link, withRouter } from 'react-router-dom';
+import { Link, RouteComponentProps, withRouter } from 'react-router-dom';
+
+interface ConnectedLinkComponentProps extends ComponentProps<any>, RouteComponentProps {
+ location: any;
+ path: string;
+ disabled: boolean;
+ query: any;
+ [key: string]: any;
+}
export function ConnectedLinkComponent({
location,
@@ -15,13 +23,7 @@ export function ConnectedLinkComponent({
disabled,
children,
...props
-}: {
- location: any;
- path: string;
- disabled: boolean;
- query: any;
- [key: string]: any;
-}) {
+}: ConnectedLinkComponentProps) {
if (disabled) {
return <EuiLink aria-disabled="true" {...props} />;
}
@@ -38,4 +40,4 @@ export function ConnectedLinkComponent({
);
}
-export const ConnectedLink = withRouter<any>(ConnectedLinkComponent);
+export const ConnectedLink = withRouter(ConnectedLinkComponent);
diff --git a/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx b/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx
index 0802b4d8ea..92dead2b70 100644
--- a/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx
+++ b/x-pack/legacy/plugins/beats_management/public/containers/with_url_state.tsx
@@ -6,7 +6,7 @@
import { parse, stringify } from 'querystring';
import React from 'react';
-import { withRouter } from 'react-router-dom';
+import { RouteComponentProps, withRouter } from 'react-router-dom';
import { FlatObject } from '../frontend_types';
import { RendererFunction } from '../utils/typed_react';
@@ -22,7 +22,7 @@ export interface URLStateProps<URLState = object> {
) => void;
urlState: URLState;
}
-interface ComponentProps<URLState extends object> {
+interface ComponentProps<URLState extends object> extends RouteComponentProps {
history: any;
match: any;
children: RendererFunction<URLStateProps<URLState>>;
@@ -86,7 +86,7 @@ export class WithURLStateComponent<URLState extends object> extends React.Compon
});
};
}
-export const WithURLState = withRouter<any>(WithURLStateComponent);
+export const WithURLState = withRouter(WithURLStateComponent);
export function withUrlState<OP>(
UnwrappedComponent: React.ComponentType<OP & URLStateProps> |
|
Yup, that's exactly what I was attempting to recommend @jfsiii ! Glad you got it to work 😊 |
package.json
Outdated
| @@ -205,6 +205,7 @@ | |||
| "node-fetch": "1.7.3", | |||
| "opn": "^5.5.0", | |||
| "oppsy": "^2.0.0", | |||
| "path-to-regexp": "^1.7.0", | |||
There was a problem hiding this comment.
We depend on this in the code project, but it's not listed in the x-pack package.json, maybe we can list it there?
| @@ -22,6 +22,7 @@ | |||
| "react": "^16.2.0", | |||
| "react-ace": "^5.9.0", | |||
| "react-color": "^2.13.8", | |||
| "react-router-dom": "^5.1.0", | |||
There was a problem hiding this comment.
this is a dev dependency for the docs site and is listed down there.
| @@ -33,6 +34,7 @@ | |||
| "@babel/core": "^7.5.5", | |||
| "@elastic/eui": "0.0.55", | |||
| "@kbn/babel-preset": "1.0.0", | |||
| "@types/react-router-dom": "^5.1.0", | |||
There was a problem hiding this comment.
This package doesn't use TS, so the types aren't necessary.
|
@cjcenizal yes the error The workaround is the one I wrote in eui: https://github.com/elastic/eui/blob/master/wiki/react-router.md#react-router-5x We can discuss later at our sync if we have time to upgrade our apps before ff. cc @jfsiii |
jfsiii
force-pushed
the
update-react-router-dom-to-v5
branch
from
e95f1a4
to
87f087e
Compare
|
@elasticmachine merge upstream |
💔 Build Failed |
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
jfsiii
force-pushed
the
update-react-router-dom-to-v5
branch
from
db3fb4b
to
9f7bc64
Compare
💔 Build Failed |
Initially got the errors described in DefinitelyTyped/DefinitelyTyped#38578 (comment) but I followed DefinitelyTyped/DefinitelyTyped#38578 (comment) and deleted the `@types/react-router` entry in `yarn.lock`
Issues are described in DefinitelyTyped/DefinitelyTyped#38271 Root cause is stated to be an issue with TS microsoft/TypeScript#34607 which has since been fixed in master. Commiting now to prevent issues, but would also like to revert this and try again when TS fix ships (TS 3.7?)
💔 Build Failed |
IIRC, I added this while tracking down the issues described in DefinitelyTyped/DefinitelyTyped#38271 It could have been another error, but we're not _supposed_ to have to add this dependency. I added it trying to suppress errors. Seeing if I can remove it and have the same results we currently have.
💔 Build Failed |
|
Closing thanks to #52445 |
jen-huang
added
the
Team:Fleet
Summary
Update
react-router-dom&@types/react-router-domto 5.x branch (5.1.1).According to the announcement for 5.0:
5.0 is also removes all warnings for
React.StrictMode5.1adds some hooks (useParams,useLocation,useHistory, etc) See the announcement for more details.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriatelyThis includes a feature addition or change that requires a release note and was labeled appropriatelyDetails about upgrading
I had to manually
When I first ran `kbn bootstrap` it failed withyarn.lockdue to this issueI removed the
"@types/react-router@*":block and re-ran `yarn add react-router-dom@^5.1.0 && yarn add -D @types/react-router-dom@^5.1.0I added
`yarn start` initially errored withpath-to-regexp@^1.7.0as an explicit dependency due to this issueI tried the
import * as pathToRegexp from 'path-to-regexp'change, but that just resulted inFATAL TypeError: pathToRegexp is not a functionThen I noticed
yarn list --pattern path-to-regexpshowed0.1.7at the root,so I ran
yarn add path-to-regexp@^1.7.0since that's the version react-router uses and the errors went away