Advanced Settings management => typescript #54477

mattkime · 2020-01-10T16:57:18Z

Summary

Conversion of advanced settings management to typescript.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] 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]~~(https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)
~~[ ] 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~~

streamich

Code LGTM, just a couple of comments about //@ts-ignore usage.

streamich · 2020-01-10T17:01:04Z

src/legacy/core_plugins/kibana/public/management/sections/settings/components/field/field.tsx

@@ -764,6 +774,7 @@ export class Field extends PureComponent {
              helpText={this.renderHelpText(setting)}
              describedByIds={[`${setting.name}-aria`]}
              className="mgtAdvancedSettings__fieldRow"
+              // @ts-ignore
              hasChildLabel={setting.type !== 'boolean'}


Can we cast here a variable to the required type, instead of using // @ts-ignore.

This is here due to incomplete eui typescript type defs.

streamich · 2020-01-10T17:03:31Z

...acy/core_plugins/kibana/public/management/sections/settings/components/component_registry.ts

+  // @ts-ignore
  if (!component.displayName) {
+    // @ts-ignore
    component.displayName = id;


Instead of disabling TypeScript compiler for these lines, is it possible to cast here?

if (!(component as any).displayName) {

Why is this actually needed ? The property seems present

interface FunctionComponent<P = {}> { (props: PropsWithChildren<P>, context?: any): ReactElement | null; propTypes?: WeakValidationMap<P>; contextTypes?: ValidationMap<any>; defaultProps?: Partial<P>; displayName?: string; }

I removed the tests for this code as its just for debugging and it was giving me some trouble.

streamich

Code LGTM, just a couple of comments about // @ts-ignore usage.

elasticmachine · 2020-01-10T17:05:13Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

mattkime · 2020-01-10T19:06:35Z

@elasticmachine merge upstream

…kibana into typescript_settings_management

pgayvallet

Nothing seems to be impacting platform code, but saw it too late, so here's some NITs 😄

pgayvallet · 2020-01-14T14:19:38Z

...acy/core_plugins/kibana/public/management/sections/settings/components/component_registry.ts

+const registry: { [key: string]: any } = {};
+type Id = string;


NIT: if Id aliases string, registry should probably be { [key: Id]: any } or even { [key: Id]: FunctionComponent }

src/legacy/core_plugins/kibana/public/management/sections/settings/types.ts

pgayvallet · 2020-01-14T14:21:57Z

...omponents/advanced_settings_voice_announcement/advanced_settings_voice_announcement.test.tsx

+const settingPartial = {
+  name: 'name',


Should this be explicitly typed?

I attempted doing this and it seemed like more work than its worth - the types are verified when the functions are called. But I might be misunderstanding something.

…ttings_management

…kibana into typescript_settings_management

mattkime · 2020-01-22T03:51:01Z

src/legacy/core_plugins/kibana/public/management/sections/settings/lib/get_val_type.ts

+import { UiSettingsType } from 'src/core/server/types';
+import { FieldSetting } from '../types';
+
+export function getValType(def: Partial<FieldSetting>, value?: any): UiSettingsType {


Changing the return type from string to UiSettingsType is causing the current test failures.

There are a number of nonconforming def values being passed. whyyyyyyy

Allowing this to return undefined seems to allow everything to work. Will take a look at master to verify this is preexisting behavior and not something I introduced.

Verified that this is behavior on master in this pr - #55551

streamich

LGTM. Only component types can be improved and would be nice to not use //@ts-ignore.

streamich · 2020-01-22T12:49:23Z

...acy/core_plugins/kibana/public/management/sections/settings/components/component_registry.ts

@@ -26,7 +29,7 @@ const registry = {};
 * @param {*} id the id of the component to register
 * @param {*} component the component
 */
-export function tryRegisterSettingsComponent(id, component) {
+export function tryRegisterSettingsComponent(id: Id, component: FunctionComponent) {


Suggested change

export function tryRegisterSettingsComponent(id: Id, component: FunctionComponent) {

export function tryRegisterSettingsComponent(id: Id, component: React.ComponentType) {

streamich · 2020-01-22T12:50:51Z

...acy/core_plugins/kibana/public/management/sections/settings/components/component_registry.ts

+import { FunctionComponent } from 'react';
+
+type Id = string;
+const registry: Record<Id, FunctionComponent> = {};


Suggested change

const registry: Record<Id, FunctionComponent> = {};

const registry: Record<Id, React.ComponentType> = {};

streamich · 2020-01-22T12:51:05Z

...acy/core_plugins/kibana/public/management/sections/settings/components/component_registry.ts

-export function registerSettingsComponent(id, component, allowOverride = false) {
+export function registerSettingsComponent(
+  id: Id,
+  component: FunctionComponent<Record<string, any>>,


Suggested change

component: FunctionComponent<Record<string, any>>,

component: ComponentType,

streamich · 2020-01-22T12:51:36Z

...acy/core_plugins/kibana/public/management/sections/settings/components/component_registry.ts

@@ -65,7 +72,7 @@ export function registerSettingsComponent(id, component, allowOverride = false)
 *
 * @param {*} id the ID of the component to retrieve
 */
-export function getSettingsComponent(id) {
+export function getSettingsComponent(id: Id): FunctionComponent<Record<string, any>> {


Suggested change

export function getSettingsComponent(id: Id): FunctionComponent<Record<string, any>> {

export function getSettingsComponent(id: Id): ComponentType {

streamich · 2020-01-22T12:52:21Z

...gacy/core_plugins/kibana/public/management/sections/settings/components/field/field.test.tsx

@@ -263,6 +276,7 @@ describe('Field', () => {
          />
        );
        const select = findTestSubject(component, `advancedSetting-editField-${setting.name}`);
+        // @ts-ignore


nit: Can we remove this and use casting, if needed?

I'm happy to do this if you can show me which type is needed as I've been unable to find a good example.

const labels = select.find('option').map((option: any) => option.text());

or

const labels = select.find('option').map((option: { text: () => string[] }) => option.text());

I'm a little confused about this. Is this useful aside from removing // @ts-ignore? Is a very incomplete type better than an ignore statement?

No, my comment was just about removing // @ts-ignore. It is just a nit.

streamich · 2020-01-22T12:52:27Z

...gacy/core_plugins/kibana/public/management/sections/settings/components/field/field.test.tsx

@@ -280,13 +294,14 @@ describe('Field', () => {
          />
        );
        const select = findTestSubject(component, `advancedSetting-editField-${setting.name}`);
+        // @ts-ignore


nit: Can we remove this and use casting, if needed?

lizozom

Tested on Ubuntu \ Chrome.
I still see some minor comments that haven't been addressed, but overall LGTM

lizozom · 2020-01-22T15:28:39Z

src/core/server/ui_settings/types.ts

@@ -88,7 +88,16 @@ export interface DeprecationSettings {
 * UI element type to represent the settings.
 * @public
 * */
-export type UiSettingsType = 'json' | 'markdown' | 'number' | 'select' | 'boolean' | 'string';
+export type UiSettingsType =
+  | 'undefined' // I don't know why malformed UiSettings objects exist


This actually makes it unneeded, right?
https://github.com/elastic/kibana/pull/55551/files

Sorry, I should have been more clear - that PR will be deleted. It was to prove that the undefined UiSettingsType is preexisting and not a result of another change in this PR

lizozom · 2020-01-22T15:29:26Z

src/core/server/ui_settings/types.ts

-export interface StringValidation {
+export type StringValidation = StringValidationRegex | StringValidationRegexString;
+
+export interface StringValidationRegex {


// nit why not create StringVlalidaiton as a compound type?

Are you asking why StringValidationRegex and StringValidationRegexString are individually exported? I do refer to them individually.

kibanamachine · 2020-01-23T15:20:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cba0e1b

History

💚 Build #22031 succeeded 1d5b66f
💔 Build #22023 failed c7c862f
💔 Build #22015 failed ee046f6
💔 Build #22010 failed 9305d05
💚 Build #22001 succeeded b229d7f

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

* advanced settings => typescript

mattkime added 3 commits January 9, 2020 09:33

partial work

afb54d8

more partial progress

e5367aa

partial progress

3038ee3

mattkime requested a review from a team as a code owner January 10, 2020 16:57

streamich approved these changes Jan 10, 2020

View reviewed changes

mattkime added the Team:AppArch label Jan 10, 2020

streamich approved these changes Jan 10, 2020

View reviewed changes

mattkime added the v8.0.0 label Jan 10, 2020

mattkime changed the title ~~Advanced Settings management => typescript~~ Advanced Settings management => typescript - WIP Jan 10, 2020

mattkime added the release_note:skip Skip the PR/issue when compiling release notes label Jan 10, 2020

elasticmachine and others added 4 commits January 10, 2020 13:06

Merge branch 'master' into typescript_settings_management

635328d

Merge branch 'master' into typescript_settings_management

d2fb793

partial progress

14aec90

Merge branch 'typescript_settings_management' of github.com:mattkime/…

4e3891e

…kibana into typescript_settings_management

mattkime added the v7.7.0 label Jan 13, 2020

Merge branch 'master' into typescript_settings_management

8e5bc1c

mattkime requested a review from a team as a code owner January 14, 2020 05:07

pgayvallet reviewed Jan 14, 2020

View reviewed changes

mattkime added 3 commits January 14, 2020 22:27

Merge branch 'master' into typescript_settings_management

3e2c4ca

update snaps, api docs

870d824

Merge branch 'master' into typescript_settings_management

e002310

This was referenced Jan 15, 2020

Management migration plan #51539

Closed

Advanced settings management UI -> TypeScript and New Platform Ready #51321

Closed

Merge branch 'master' into typescript_settings_management

f99f7fe

mattkime requested a review from a team as a code owner January 15, 2020 17:25

partial progress

863e535

mattkime added 6 commits January 21, 2020 19:52

supply type for default index

6e39dd2

Merge branch 'master' of github.com:elastic/kibana into typescript_se…

3fc125c

…ttings_management

morer api docs

d07728f

Merge branch 'typescript_settings_management' of github.com:mattkime/…

9b39c3d

…kibana into typescript_settings_management

better logging for incompatible setting types

fcd5d2b

better logging for incompatible setting types

bc055cc

mattkime commented Jan 22, 2020

View reviewed changes

mattkime added 3 commits January 21, 2020 22:16

allow undefined UiSettingsType

2b0c9c2

allow undefined UiSettingsType

4f1d872

update api docs

05a85f2

streamich approved these changes Jan 22, 2020

View reviewed changes

mattkime added 10 commits January 22, 2020 08:57

better react component types

a3ad2f9

move types to common location

e2d56c3

shared location for saved object types

77c741e

shared location for saved object types

161d4f1

update docs

5a54ac4

update docs

b229d7f

use queryText as appropriate

9305d05

Merge branch 'master' into typescript_settings_management

ee046f6

fix advanced settings test

c7c862f

update snapshot

1d5b66f

lizozom approved these changes Jan 23, 2020

View reviewed changes

Merge branch 'master' into typescript_settings_management

cba0e1b

mattkime merged commit 0c25cb5 into elastic:master Jan 23, 2020

mattkime mentioned this pull request Jan 23, 2020

[7.x] Advanced Settings management => typescript (#54477) #55716

Merged

mattkime added a commit to mattkime/kibana that referenced this pull request Jan 23, 2020

Advanced Settings management => typescript (elastic#54477)

0af2458

* advanced settings => typescript

mattkime added a commit that referenced this pull request Jan 23, 2020

Advanced Settings management => typescript (#54477) (#55716)

ced9f76

* advanced settings => typescript

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced Settings management => typescript #54477

Advanced Settings management => typescript #54477

mattkime commented Jan 10, 2020 •

edited

Loading

streamich left a comment

streamich Jan 10, 2020

mattkime Jan 19, 2020

streamich Jan 10, 2020

pgayvallet Jan 14, 2020

mattkime Jan 19, 2020 •

edited

Loading

streamich left a comment •

edited

Loading

elasticmachine commented Jan 10, 2020

mattkime commented Jan 10, 2020

pgayvallet left a comment

pgayvallet Jan 14, 2020 •

edited

Loading

mattkime Jan 19, 2020

pgayvallet Jan 14, 2020

mattkime Jan 19, 2020

mattkime Jan 22, 2020 •

edited

Loading

mattkime Jan 22, 2020

mattkime Jan 22, 2020

streamich left a comment •

edited

Loading

streamich Jan 22, 2020

streamich Jan 22, 2020 •

edited

Loading

streamich Jan 22, 2020

streamich Jan 22, 2020

streamich Jan 22, 2020

mattkime Jan 22, 2020

streamich Jan 22, 2020

mattkime Jan 22, 2020 •

edited

Loading

streamich Jan 22, 2020

streamich Jan 22, 2020

lizozom left a comment

lizozom Jan 22, 2020 •

edited

Loading

mattkime Jan 23, 2020

lizozom Jan 22, 2020

mattkime Jan 23, 2020

kibanamachine commented Jan 23, 2020

		const registry: { [key: string]: any } = {};
		type Id = string;

	export function tryRegisterSettingsComponent(id: Id, component: FunctionComponent) {
	export function tryRegisterSettingsComponent(id: Id, component: React.ComponentType) {

	const registry: Record<Id, FunctionComponent> = {};
	const registry: Record<Id, React.ComponentType> = {};

	component: FunctionComponent<Record<string, any>>,
	component: ComponentType,

	export function getSettingsComponent(id: Id): FunctionComponent<Record<string, any>> {
	export function getSettingsComponent(id: Id): ComponentType {

Advanced Settings management => typescript #54477

Advanced Settings management => typescript #54477

Conversation

mattkime commented Jan 10, 2020 • edited Loading

Summary

Checklist

For maintainers

streamich 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

mattkime Jan 19, 2020 • edited Loading

Choose a reason for hiding this comment

streamich left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jan 10, 2020

mattkime commented Jan 10, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

pgayvallet Jan 14, 2020 • 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 Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

streamich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

streamich Jan 22, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

lizozom Jan 22, 2020 • 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

kibanamachine commented Jan 23, 2020

💚 Build Succeeded

History

mattkime commented Jan 10, 2020 •

edited

Loading

mattkime Jan 19, 2020 •

edited

Loading

streamich left a comment •

edited

Loading

pgayvallet Jan 14, 2020 •

edited

Loading

mattkime Jan 22, 2020 •

edited

Loading

streamich left a comment •

edited

Loading

streamich Jan 22, 2020 •

edited

Loading

mattkime Jan 22, 2020 •

edited

Loading

lizozom Jan 22, 2020 •

edited

Loading