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

Prepare for TS 3.7 upgrade #47794

Merged
merged 2 commits into from
Oct 14, 2019
Merged

Prepare for TS 3.7 upgrade #47794

merged 2 commits into from
Oct 14, 2019

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Oct 10, 2019

Summary

This PR addresses a couple of TS issues, that will break once upgraded to TS 3.7.0. This PR mostly contains smaller changes not really effecting any specific plugin a lot, so I randomly select a couple of TS experts for review here.

For QA: This PR does not contain any functional changes.

@timroes timroes added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 10, 2019
@timroes timroes requested a review from a team as a code owner October 10, 2019 07:40
@timroes timroes requested a review from a team October 10, 2019 07:40
@@ -86,7 +86,7 @@ export class DashboardContainerExample extends React.Component<Props, State> {
}

public switchViewMode = () => {
this.setState((prevState: State) => {
this.setState<'viewMode'>((prevState: State) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ It complains that { viewMode } is missing loading, because I think it cannot automatically infer the generic type here, due to the if (it actually infers it to never).

@@ -61,7 +61,7 @@ function isWhitelistedHostnameInUri(config: ActionsConfigType, uri: string): boo
tryCatch(() => new URL(uri)),
map(url => url.hostname),
mapNullable(hostname => isWhitelisted(config, hostname)),
getOrElse(() => false)
getOrElse<boolean>(() => false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ It infers the generic type false here, and then complains, that the mapNullable produces a boolean which is not assignable to false. I think the typings of pipe are just a bit weird, that they are "checking backwards" from the last function.

@@ -95,4 +95,4 @@ export interface AlertingPlugin {
start: PluginStartContract;
}

export type AlertTypeRegistry = PublicMethodsOf<AlertTypeRegistry>;
export type AlertTypeRegistry = PublicMethodsOf<OrigAlertTypeRegistry>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ The usual 3.7 issue, that you cannot have an import and export with the same name (but different values) in one file anymore.

@@ -96,6 +96,7 @@ declare module '@elastic/eui' {
message?: any;
rowProps?: any;
cellProps?: any;
responsive?: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This was just used somewhere, and I honestly have no idea how this didn't fail in the current TS .

Object.keys(IndexGroup).forEach(typeKey => {
const consumerType = IndexGroup[typeKey as any] as IndexGroup;

Object.entries(IndexGroup).forEach(([typeKey, consumerType]) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ The as any typing complained, that any is not a valid key to the IndexGroup enum. So I actually just used the entries method here, which a/ looks nicer and b/ is perfectly type safe now.

@timroes timroes mentioned this pull request Oct 10, 2019
15 tasks
@timroes
Copy link
Contributor Author

timroes commented Oct 11, 2019

Jenkins, test this - CI failed to report

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor Author

timroes commented Oct 14, 2019

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor Author

timroes commented Oct 14, 2019

Ignoring CODEOWNERS review since it only changing very minor typings in their files.

@timroes timroes merged commit 679bce1 into elastic:master Oct 14, 2019
@timroes timroes deleted the ts-cleanup-misc branch October 14, 2019 20:23
TinaHeiligers pushed a commit to TinaHeiligers/kibana that referenced this pull request Oct 14, 2019
timroes pushed a commit that referenced this pull request Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants