[Data Views] Remove remaining "any" usage #136343

tsullivan · 2022-07-13T20:25:18Z

Summary

Follows #135767

This removes the remaining any from production code and code comments in src/plugins/data_v*

Note: some instances have remained due to ambiguity in the code, but these have had added FIXME comments:

After this PR, the remaining matches of the term any exist in the following:

the various added FIXMEs
unit test code
user-facing content strings that includes the string any

Remaining any matches:

…-usage-ii

elasticmachine · 2022-07-14T01:17:42Z

Pinging @elastic/kibana-app-services (Team:AppServicesUx)

nreese

kibana-gis changes LGTM
code review

…e-ii

tsullivan · 2022-07-14T14:21:15Z

src/plugins/data_views/public/debounce_by_key.ts

-  waitInMs: number
-): ((key: string) => F) => {
-  const debouncerCollector: Record<string, F> = {};
+export const debounceByKey = <F extends (...args: any) => unknown>(fn: F, waitInMs: number) => {


I don't think there's a way to get rid of this any

Might be good to leave a code comment

tsullivan · 2022-07-14T14:22:32Z

src/plugins/data_views/server/fetcher/index.ts

 } from './lib';
+export type { RollupIndexCapability } from './lib';


This needs to be exported since its part of a public API

Could you document it? Trying to prevent Public APIs missing comments from increasing

tsullivan · 2022-07-14T14:24:42Z

src/plugins/data_views/server/fetcher/lib/jobs_compatibility.ts

-              const isMultiple = intervals[1] % intervals[0] === 0;
-              fieldAgg.interval = isMultiple ? intervals[1] : intervals[0] * intervals[1];
+              if (fieldAgg) {
+                const aggInterval = (agg as any).interval ?? agg.calendar_interval; // FIXME the interface infers only that calendar_interval property is valid


I couldn't find an interval field on the interface that is expected here, I thought maybe it should be calendar_interval -- or maybe the typings are not complete for the histogram agg

tsullivan · 2022-07-14T14:32:35Z

x-pack/plugins/lens/public/indexpattern_datasource/types.ts

@@ -7,7 +7,7 @@

 import type { IndexPatternAggRestrictions } from '@kbn/data-plugin/public';
 import type { FieldSpec } from '@kbn/data-plugin/common';
-import type { FieldFormatParams } from '@kbn/field-formats-plugin/common';
+import type { FieldFormatMap } from '@kbn/data-views-plugin/common';


I'm not sure why FieldFormat and FieldFormatParams are exported from the field-formats plugin,

but FieldFormatMap is exported from the data-views plugin. 😕

FieldFormatMap is how data views saves field formatters

tsullivan · 2022-07-14T14:45:15Z

x-pack/plugins/lens/public/indexpattern_datasource/loader.ts

@@ -92,7 +93,7 @@ export function convertDataViewIntoLensIndexPattern(dataView: DataView): IndexPa
      Object.fromEntries(
        Object.entries(fieldFormatMap).map(([id, format]) => [
          id,
-          'toJSON' in format ? format.toJSON() : format,
+          'toJSON' in format ? (format as unknown as FieldFormat).toJSON() : format, // FIXME SerializedFieldFormat was inferred... was this intended to work with FieldFormat instead?


I need reviewers to look closely at this: I could not see how there would be a .toJSON property on this object.

mattkime

A couple of notes otherwise great work!

mattkime · 2022-07-14T15:46:45Z

src/plugins/data_views/common/fields/utils.ts

 */
-export function shortenDottedString(input: any) {
-  return typeof input !== 'string' ? input : input.replace(DOT_PREFIX_RE, '$1.');
+export function shortenDottedString(input: unknown): string {


When is this called on something aside from a string?

it was only in unit tests

mattkime · 2022-07-14T16:00:06Z

x-pack/plugins/lens/public/indexpattern_datasource/field_item.tsx

@@ -432,7 +432,7 @@ function FieldItemPopoverContents(props: State & FieldItemProps) {

  let formatter: { convert: (data: unknown) => string };
  if (indexPattern.fieldFormatMap && indexPattern.fieldFormatMap[field.name]) {
-    const FormatType = fieldFormats.getType(indexPattern.fieldFormatMap[field.name].id);
+    const FormatType = fieldFormats.getType(indexPattern.fieldFormatMap[field.name].id as string);


Perhaps fieldFormats.getType needs better type info instead of using as string on the id

The change in this PR is to type-guard that indexPattern.fieldFormatMap[field.name].id is a string, because an argument to FieldFormatsRegistry.getType is required: https://github.com/elastic/kibana/blob/8.3/src/plugins/field_formats/common/field_formats_registry.ts#L92

~~Hm, checking against main, the "fieldFormats" object was typed as FieldFormatsStart not FieldFormatsRegistry. Need to look into this.~~

Edit: this change came from the type refinement where indexPattern.fieldFormatMap was typed as:

(property) IndexPattern.fieldFormatMap?: Record<string, { id: string; params: SerializableRecord; }>

But is now: IndexPattern.fieldFormatMap?: FieldFormatMap, which types to Record<string, SerializedFieldFormat>, and types to this:

export type SerializedFieldFormat< P = {}, TParams extends FieldFormatParams<P> = FieldFormatParams<P> > = { id?: string; params?: TParams; };

With the refined types, id is now inferred to be optional. The previous typing from this code assumed id and params is not optional: https://github.com/elastic/kibana/blob/8.3/x-pack/plugins/lens/public/indexpattern_datasource/types.ts#L59-L60

I have zero understanding what consequences this might bring but it would be nice if id could be required. I guess params too if that works with existing code. Perhaps an empty object is always provided.

Looks like the object could come from user input, and the ID is not validated to be present: https://github.com/elastic/kibana/blob/8.3/src/plugins/data_views/server/rest_api_routes/util/schemas.ts#L13.

There is quite a lot of test code that relies on these fields being optional as well.

tsullivan · 2022-07-15T02:02:34Z

It looks like the code metric script finds that DataViewSavedObjectAttrs is an uncommented public API. I'm not sure why it's doing that, since the needed comment seems to be there.

cla-checker-service · 2022-07-15T20:40:59Z

💚 CLA has been signed

…-usage-ii

flash1293 · 2022-07-19T12:22:31Z

x-pack/plugins/lens/public/indexpattern_datasource/loader.ts

@@ -100,6 +100,7 @@ export function convertDataViewIntoLensIndexPattern(dataView: DataView): IndexPa
      Object.fromEntries(
        Object.entries(fieldFormatMap).map(([id, format]) => [
          id,
+          // @ts-expect-error FIXME Property 'toJSON' does not exist on type 'SerializedFieldFormat'


@dej611 I remember we were struggling a bit with this - do you know where this is coming from? How can there be a toJSON on the format that should be serialized? Was this done to work around some bug?

flash1293

VisEditors changes LGTM

…-usage-ii

mattkime

changes lgtm - nice type improvements!

kibana-ci · 2022-07-19T18:24:02Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`dataViews`	202	203	+1

Any counts in public APIs

Total count of every any typed public API. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats any for more detailed information.

id	before	after	diff
`dataViews`	1	0	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`visTypeTimeseries`	452.0KB	452.0KB	+57.0B

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`dataViews`	1	0	-1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`dataViews`	40.7KB	40.8KB	+83.0B

Unknown metric groups

API count

id	before	after	diff
`dataViews`	942	945	+3

ESLint disabled line counts

id	before	after	diff
`dataViews`	10	9	-1

Total ESLint disabled count

id	before	after	diff
`dataViews`	12	11	-1

History

💚 Build #58640 succeeded d3c66b3
💚 Build #58029 succeeded 2a2d14a
💚 Build #57979 succeeded 250d034
💚 Build #57911 succeeded b7a43ec
💔 Build #57864 failed 524027e
💔 Build #57850 failed fbad73a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

tsullivan added 9 commits July 13, 2022 10:20

fix some any

deead9c

remove some any

8ff1b10

fix some any in saved_objects_client_wrapper

f1eed72

fix some any in common/types

ad9b69d

remove some any from flattenHit and getFields*

700bdea

fix more any

b3a071a

remove "any" in code comments

7604fe2

fix types

266f750

test commit

bf3d887

tsullivan force-pushed the dataviews/remove-any-usage-ii branch from 4acfa7d to bf3d887 Compare July 13, 2022 20:58

tsullivan added 8 commits July 13, 2022 14:04

Merge remote-tracking branch 'elastic/main' into dataviews/remove-any…

b0b7817

…-usage-ii

fix ts

366c2f8

fix types

40a62ed

remove more any

5ee6bd6

Merge remote-tracking branch 'elastic/main' into dataviews/remove-any…

7bcd468

…-usage-ii

simplify / comment

daf68f9

more simplification

7712b06

fix test

b7d44a6

tsullivan marked this pull request as ready for review July 14, 2022 01:16

tsullivan requested review from a team as code owners July 14, 2022 01:16

tsullivan added Feature:Data Views Data Views code and UI - index patterns before 8.0 release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:AppServicesUx v8.4.0 labels Jul 14, 2022

tsullivan marked this pull request as draft July 14, 2022 01:19

export public APIs

d3dbc00

nreese approved these changes Jul 14, 2022

View reviewed changes

Merge remote-tracking branch 'elastic' into dataviews/remove-any-usag…

043a4e5

…e-ii

tsullivan marked this pull request as ready for review July 14, 2022 14:17

tsullivan commented Jul 14, 2022

View reviewed changes

mattkime requested changes Jul 14, 2022

View reviewed changes

tsullivan added 7 commits July 14, 2022 09:31

improve documentation and types for RollupIndexCapability

fbad73a

simplify shortenDottedString

524027e

fix jest test

b7a43ec

use AggregationRestrictions

90b995c

fix downstream types

275e7a3

simplify

35c4dcf

add comments for public api items

250d034

tsullivan force-pushed the dataviews/remove-any-usage-ii branch from 3cca8c7 to 250d034 Compare July 15, 2022 20:49

tsullivan added 2 commits July 15, 2022 13:53

add ts-expect-error in lens integration toJSON

9db3690

Merge remote-tracking branch 'elastic/main' into dataviews/remove-any…

2a2d14a

…-usage-ii

tsullivan force-pushed the dataviews/remove-any-usage-ii branch from c9de913 to 2a2d14a Compare July 15, 2022 20:53

Merge branch 'main' into dataviews/remove-any-usage-ii

d3c66b3

flash1293 reviewed Jul 19, 2022

View reviewed changes

flash1293 approved these changes Jul 19, 2022

View reviewed changes

Merge remote-tracking branch 'elastic/main' into dataviews/remove-any…

d525db7

…-usage-ii

mattkime approved these changes Jul 19, 2022

View reviewed changes

tsullivan merged commit a2c3323 into elastic:main Jul 19, 2022

tsullivan deleted the dataviews/remove-any-usage-ii branch July 29, 2022 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Data Views] Remove remaining "any" usage #136343

[Data Views] Remove remaining "any" usage #136343

tsullivan commented Jul 13, 2022 •

edited

Loading

elasticmachine commented Jul 14, 2022

nreese left a comment

tsullivan Jul 14, 2022

mattkime Jul 14, 2022

tsullivan Jul 14, 2022

mattkime Jul 14, 2022

tsullivan Jul 14, 2022 •

edited

Loading

tsullivan Jul 14, 2022

mattkime Jul 14, 2022

tsullivan Jul 14, 2022

mattkime left a comment

mattkime Jul 14, 2022

tsullivan Jul 14, 2022

mattkime Jul 14, 2022

tsullivan Jul 14, 2022 •

edited

Loading

mattkime Jul 14, 2022

tsullivan Jul 15, 2022

tsullivan commented Jul 15, 2022

cla-checker-service bot commented Jul 15, 2022 •

edited

Loading

flash1293 Jul 19, 2022

flash1293 left a comment

mattkime left a comment

kibana-ci commented Jul 19, 2022

API count

ESLint disabled line counts

Total ESLint disabled count

		} from './lib';
		export type { RollupIndexCapability } from './lib';

[Data Views] Remove remaining "any" usage #136343

[Data Views] Remove remaining "any" usage #136343

Conversation

tsullivan commented Jul 13, 2022 • edited Loading

Summary

elasticmachine commented Jul 14, 2022

nreese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan commented Jul 15, 2022

cla-checker-service bot commented Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 19, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Any counts in public APIs

Async chunks

Public APIs missing exports

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

tsullivan commented Jul 13, 2022 •

edited

Loading

tsullivan Jul 14, 2022 •

edited

Loading

tsullivan Jul 14, 2022 •

edited

Loading

cla-checker-service bot commented Jul 15, 2022 •

edited

Loading