Update Typescript to the latest version #32063

mshustov · 2019-02-26T17:52:08Z

Summary

Working on issue #31486, after finishing #31825 I realised that node scripts/type_check indicates some type errors, while VSCode with the latest TS does not detect any problems.

Checklist

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-02-26T18:37:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-26T18:38:45Z

Pinging @elastic/kibana-platform

elasticmachine · 2019-02-26T18:38:47Z

Pinging @elastic/kibana-operations

spalger

One thing, but LGTM otherwise

spalger · 2019-02-26T23:34:36Z

src/legacy/ui/public/vis/update_status.ts

@@ -59,8 +59,8 @@ function getUpdateStatus<T extends Status>(
  requiresUpdateStatus: T[] = [],
  obj: any,
  param: { vis: Vis; visData: any; uiState: PersistedState }
-): { [reqStats in T]: boolean } {
-  const status = {} as { [reqStats in T]: boolean };
+): { [reqStats in Status]: boolean } {


Took a double look at this, I think we'd be better off getting rid of the T extends Status bit and just using Status in place of T everywhere.

function getUpdateStatus( requiresUpdateStatus: Status[] = [], obj: any, param: { vis: Vis; visData: any; uiState: PersistedState } ): { [reqStats in Status]: boolean } { const status = {} as { [reqStats in Status]: boolean }; ...

@timroes do you remember why this uses T extends Status instead of just referencing the Status enum directly? The indirection seems to have worked in the past but is now confusing TypeScript which complains about the assignments to status.aggs and such below:

Because we know that the return object doesn't have all possible values of the Status enum as a key, so reqStats in Status is simply wrong. The method signature was in a way so that you pass in an array of enum values as a parameter and the result object will have exactly only those enum values as keys (not more not less from the enum) and boolean value for each of them.

Earlier the correct syntax for that was like that. [reqStats in Status] is definitely the wrong typing, since it would say the result object has ALWAYS all the enum values as keys, which it doesn't. I can try to look into that today or tomorrow to find a better solution, that will still break. The least thing that should still work is { [reqStats in Status]?: boolean } which allows each of the keys to be undefined. Since we're losing type information like that, that we're currently having I would like to check if there is another better way to solve that.

I tested it. You can simply revert that line again. It's enough if you instantiate the status object in the next line as { [reqStats in Status]: boolean } (as you already do). That line can stay the same. And TypeScript won't complain anymore, but will still have the nice syntax for the caller, that the return object will only contain the requested keys.

timroes

The vis_update typing is currently broken. Will try to help out as soon as I can to find a better solution.

Found a solution for the vis_update problem, lifting my review assuming this will be fixed.

x-pack/plugins/apm/public/components/app/ServiceDetails/ServiceIntegrations/WatcherFlyout.tsx

mshustov · 2019-02-27T12:21:26Z

x-pack/plugins/infra/public/components/range_date_picker/index.tsx

@@ -255,6 +255,7 @@ export const RangeDatePicker = injectI18n(
          id="QuickSelectPopover"
          button={quickSelectButton}
          isOpen={this.state.isPopoverOpen}
+          // @ts-ignore


here callback is defined as private closePopover = (type: string, from?: string, to?: string) => {
but closePopover in <EuiPopover doesn't pass any arguments. tried to fix it with making all params optional

private closePopover = (type?: string, from?: string, to?: string) => {

but it leads to other errors in this file, so suppressed for now

mshustov · 2019-02-27T12:23:58Z

x-pack/plugins/infra/public/containers/with_state_from_location.tsx

@@ -50,6 +50,7 @@ export const withStateFromLocation = <StateInLocation extends {}>({
        const stateFromLocation = mapLocationToState(location);

        return (
+          // @ts-ignore


Probably author could help with this error

Type 'Readonly<{ children?: ReactNode; }> & Readonly<RouteComponentProps<{}, StaticContext, any> & Pick<WrappedComponentProps, Exclude<keyof WrappedComponentProps, keyof StateInLocation | "pushStateInLocation" | "replaceStateInLocation">>> & StateInLocation & { ...; }' is not assignable to type 'IntrinsicAttributes & WrappedComponentProps & { children?: ReactNode; }'. Type 'Readonly<{ children?: ReactNode; }> & Readonly<RouteComponentProps<{}, StaticContext, any> & Pick<WrappedComponentProps, Exclude<keyof WrappedComponentProps, keyof StateInLocation | "pushStateInLocation" | "replaceStateInLocation">>> & StateInLocation & { ...; }' is not assignable to type 'WrappedComponentProps'.

elasticmachine · 2019-02-27T13:18:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sorenlouv

👍 from APM

kobelb · 2019-02-27T17:27:03Z

x-pack/plugins/spaces/public/views/nav_control/components/spaces_menu.tsx

+              defaultMessage: 'Find a space',
+            })}
+            incremental={true}
+            // FIXME needs updated typedef


We used to be able to put the // @ts-ignore directly above the invalid prop, which meant if we had another invalid prop we'd get a TSC error. Moving this to on top of the EuiFieldSearch allows the entire element to be invalid. Do you know why we can't do the previous approach any longer?

@kobelb
unfortunately not. They has changed reporting mechanism for missing property error microsoft/TypeScript#26423

I'm not finding any alternatives at this point in time, but it does eliminate the targeting ignoring that we used to be able to perform, which means we really shouldn't be relying on ts-ignore in these situations. I'll open a PR to fix the type defs in EUI and get rid of these in a subsequent PR.

@ts-ignore

* bump typescript version to 3.3.3333 * fix tests after updating TS version * suppress type errors until they fixed appropriately * address comments * add type def for UnconnectedKibanaLink * remove fix @ts-ignore * fix snapshot test. provide displayName

didn't merge latest master in elastic#32063

@ts-ignore

* bump typescript version to 3.3.3333 * fix tests after updating TS version * suppress type errors until they fixed appropriately * address comments * add type def for UnconnectedKibanaLink * remove fix @ts-ignore * fix snapshot test. provide displayName

didn't merge latest master in #32063

@ts-ignore

* Update Typescript to the latest version (#32063) * bump typescript version to 3.3.3333 * fix tests after updating TS version * suppress type errors until they fixed appropriately * address comments * add type def for UnconnectedKibanaLink * remove fix @ts-ignore * fix snapshot test. provide displayName * update lost test

mshustov added 3 commits February 26, 2019 18:39

bump typescript version to 3.3.3333

5052198

fix tests after updating TS version

bc26cd3

suppress type errors until they fixed appropriately

a926a2f

mshustov changed the title ~~Update Typescript to latest version~~ Update Typescript to the latest version Feb 26, 2019

mshustov marked this pull request as ready for review February 26, 2019 18:38

mshustov requested review from a team as code owners February 26, 2019 18:38

mshustov added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Operations Team label for Operations Team labels Feb 26, 2019

mshustov added the non-issue Indicates to automation that a pull request should not appear in the release notes label Feb 26, 2019

spalger approved these changes Feb 26, 2019

View reviewed changes

mshustov added the chore label Feb 27, 2019

timroes previously requested changes Feb 27, 2019

View reviewed changes

address comments

e43128c

sorenlouv reviewed Feb 27, 2019

View reviewed changes

x-pack/plugins/apm/public/components/app/ServiceDetails/ServiceIntegrations/WatcherFlyout.tsx Outdated Show resolved Hide resolved

mshustov added 2 commits February 27, 2019 13:00

add type def for UnconnectedKibanaLink

838d1bf

remove fix @ts-ignore

631fbd2

mshustov commented Feb 27, 2019

View reviewed changes

fix snapshot test. provide displayName

e27beb5

mshustov requested review from sorenlouv and timroes and removed request for sorenlouv February 27, 2019 13:20

sorenlouv approved these changes Feb 27, 2019

View reviewed changes

mshustov mentioned this pull request Feb 27, 2019

Mocks at the service level #31486

Closed

8 tasks

kobelb reviewed Feb 27, 2019

View reviewed changes

kobelb approved these changes Feb 28, 2019

View reviewed changes

mshustov merged commit 8c6b1e5 into elastic:master Feb 28, 2019

mshustov deleted the issue-31486-update-ts branch February 28, 2019 16:21

mshustov mentioned this pull request Feb 28, 2019

[7.x] Update Typescript to the latest version (#32063) #32215

Merged

mshustov added a commit to mshustov/kibana that referenced this pull request Feb 28, 2019

fix typings for a test

87e1b99

didn't merge latest master in elastic#32063

This was referenced Feb 28, 2019

fix typings for a test #32217

Merged

[7.0] Update Typescript to the latest version (#32063) #32219

Closed

mshustov added a commit that referenced this pull request Feb 28, 2019

fix typings for a test (#32217)

e4e3354

didn't merge latest master in #32063

rudolf mentioned this pull request Mar 4, 2019

Generate core API docs from TSDoc comments #32148

Merged

9 tasks

mshustov added the backported label Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Typescript to the latest version #32063

Update Typescript to the latest version #32063

mshustov commented Feb 26, 2019

elasticmachine commented Feb 26, 2019

elasticmachine commented Feb 26, 2019

elasticmachine commented Feb 26, 2019

spalger left a comment

spalger Feb 26, 2019 •

edited

Loading

timroes Feb 27, 2019

timroes Feb 27, 2019

timroes left a comment

mshustov Feb 27, 2019

mshustov Feb 27, 2019

elasticmachine commented Feb 27, 2019

sorenlouv left a comment

kobelb Feb 27, 2019 •

edited

Loading

mshustov Feb 28, 2019 •

edited

Loading

kobelb Feb 28, 2019

Update Typescript to the latest version #32063

Update Typescript to the latest version #32063

Conversation

mshustov commented Feb 26, 2019

Summary

Checklist

For maintainers

elasticmachine commented Feb 26, 2019

💚 Build Succeeded

elasticmachine commented Feb 26, 2019

elasticmachine commented Feb 26, 2019

spalger left a comment

Choose a reason for hiding this comment

spalger Feb 26, 2019 • edited Loading

Choose a reason for hiding this comment

timroes Feb 27, 2019

Choose a reason for hiding this comment

timroes Feb 27, 2019

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

mshustov Feb 27, 2019

Choose a reason for hiding this comment

mshustov Feb 27, 2019

Choose a reason for hiding this comment

elasticmachine commented Feb 27, 2019

💚 Build Succeeded

sorenlouv left a comment

Choose a reason for hiding this comment

kobelb Feb 27, 2019 • edited Loading

Choose a reason for hiding this comment

mshustov Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

kobelb Feb 28, 2019

Choose a reason for hiding this comment

spalger Feb 26, 2019 •

edited

Loading

kobelb Feb 27, 2019 •

edited

Loading

mshustov Feb 28, 2019 •

edited

Loading