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.

Sign up for GitHub

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

Jump to bottom

[wip] typescript dashboard stuff #27167

Merged
merged 8 commits into from
Mar 4, 2019
Merged

[wip] typescript dashboard stuff #27167

merged 8 commits into from
Mar 4, 2019

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Dec 13, 2018

No description provided.

@rshen91 rshen91 added WIP Work in progress v6.6.0 labels Dec 13, 2018
@rshen91 rshen91 mentioned this pull request Dec 13, 2018
Closed
@rshen91 rshen91 added Feature:Dashboard Dashboard related features v7.0.0 labels Dec 13, 2018
@elasticmachine
Copy link
Contributor

💔 Build Failed

stacey-gammon
stacey-gammon reviewed Dec 14, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
) => {};
embeddableIsInitializing: EmbeddableActions;
embeddableIsInitialized: (
embeddableIsInitialized: import('ui/embeddable/embeddable').EmbeddableMetadata
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the intake ci is choking on these inline imports. I know we fixed them, but then ended up throwing the changes away and didn't go back around to it.

16:51:13 [21:51:13] →  I18N ERROR  Error in src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx
16:51:13 Error: Unexpected token (42:28):
16:51:13   embeddableFactory: EmbeddableFactory;
16:51:13   embeddableStateChanged: (
16:51:13     embeddableStateChanges: import('ui/embeddable/types').EmbeddableState
16:51:13   ) => {};
16:51:13   embeddableIsInitializing: EmbeddableActions;
16:51:13 ERROR  I18N ERROR  Error in src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx
16:51:13       Error: Unexpected token (42:28):
16:51:13         embeddableFactory: EmbeddableFactory;
16:51:13         embeddableStateChanged: (
16:51:13           embeddableStateChanges: import('ui/embeddable/types').EmbeddableState
16:51:13         ) => {};
16:51:13         embeddableIsInitializing: EmbeddableActions;
16:51:13 Warning: non-zero exit code 1� Use --force to continue.
16:51:13

Look at the intake failures first, it's possible something being caught in there is causing the later selenium failures as well, but the error output on the intake one is easier to pinpoint what exactly the issue is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! I think I was able to rebase the commits from my feature branch over that should have the import changes in dashboard panel.

@rshen91 rshen91 force-pushed the rshen-dev-ts branch 2 times, most recently from b24f897 to 37be13d Compare December 14, 2018 17:01
@elasticmachine
Copy link
Contributor

💔 Build Failed

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
interface DashboardPanelProps {
intl: any;
viewOnlyMode: boolean;
onPanelFocused: (panel: string) => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onPanelFocused: (panel: string) => {};
onPanelFocused?: (panel: string) => {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a ton for looking this over. Adding this change in my branch since it's not causing any new problems by making this optional.

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
intl: any;
viewOnlyMode: boolean;
onPanelFocused: (panel: string) => {};
onPanelBlurred: (panel: string) => {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onPanelBlurred: (panel: string) => {};
onPanelBlurred?: (panel: string) => {};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! Changing this to optional since it's not causing any additional problems.

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
viewOnlyMode: boolean;
onPanelFocused: (panel: string) => {};
onPanelBlurred: (panel: string) => {};
error: string | object;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
error: string | object;
error?: string | object;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing this to optional results in the following errors that were then resolved when marking
error?: string | object; in line 150 as well

"message": "Argument of type 'typeof DashboardPanelUi' is not assignable to parameter of type 'ComponentType'.\n Type 'typeof DashboardPanelUi' is not assignable to type 'ComponentClass'.\n Type 'DashboardPanelUi' is not assignable to type 'Component<DashboardPanelProps, ComponentState, any>'.\n Types of property 'shouldComponentUpdate' are incompatible.\n Type '(nextProps: { containerState: ContainerState; error: string | object; initialized: boolean; }) => boolean' is not assignable to type '(nextProps: Readonly, nextState: Readonly, nextContext: any) => boolean'.\n Types of parameters 'nextProps' and 'nextProps' are incompatible.\n Type 'Readonly' is not assignable to type '{ containerState: ContainerState; error: string | object; initialized: boolean; }'.",
"source": "ts",
"startLineNumber": 198,
"startColumn": 42,
"endLineNumber": 198,
"endColumn": 58
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You should actually use the same interface for nextProps, it now has conflicting types which is causing the error. so instead of

nextProps: {
     containerState: ContainerState;
     error: string | object;
     initialized: boolean;
   })

do

nextProps: DashboardPanelProps)

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
panel: PanelState;
isPopoverOpen: boolean;
isLoading: boolean;
calloutTitle: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this one is interesting, I don't see if used anywhere, nor in the old props. Any reason you added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed
isPopoverOpen: boolean; isLoading: boolean; calloutTitle: string; in my branch and it's not causing any issues. I might have added these by accident earlier...

Removing panel: PanelState; causes some errors (line 82, 115, 122, 170)

Thank you so much for looking this over

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, keep panel, it's used.

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
embeddableError: EmbeddableErrorAction;
initialized: boolean;
panel: PanelState;
isPopoverOpen: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

this one also looks unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome - removed embeddableError and isPopoverOpen. The panel and initialized cause errors when I remove them here so I left them.

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx
onPanelBlurred: (panel: string) => {};
error: string | object;
destroy: () => void;
containerState: ContainerState;
Copy link
Contributor

Choose a reason for hiding this comment

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

According to original code, this one is optional too.

Suggested change
containerState: ContainerState;
containerState?: ContainerState;

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 is causing ts problems when marked as optional in terms of lines this.embeddable.render(this.panelElement, this.props.containerState) and
if (this.embeddable && !_.isEqual(nextProps.containerState, this.props.containerState))

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
initialized: boolean;
panel: PanelState;
isPopoverOpen: boolean;
isLoading: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed isPopoverOpen and isLoading thanks!

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx
embeddableIsInitializing();
embeddableFactory.create(panel, embeddableStateChanged)
.then((embeddable) => {
embeddableFactory
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you dropped the first call to embeddableIsInitializing();. intentional? Might be the reason for the failing tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a ton! I was struggling on an error when adding this back in but recently figured that out. Changed the code to include embeddableIsInitializing() and changed line 48 to embeddableIsInitializing: () => void;

stacey-gammon
stacey-gammon reviewed Dec 18, 2018
View reviewed changes
src/legacy/core_plugins/kibana/public/dashboard/panel/dashboard_panel.tsx Outdated
if (this.mounted) {
embeddableError(error.message);
this.embeddableError = error.message;
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think you want to change this to a local variable, the function was setting in the redux state tree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok thanks! Changed it back

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@rshen91 rshen91 requested a review from mattkime March 4, 2019 16:15
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

rshen91 added 8 commits March 4, 2019 11:02
@rshen91
typescript dashboard stuff
d2f9003 
fixed imports in dpc

consitency across id -> i for gridData, improvment made in dashboard_panel.tsx

changed DashboardPanelUiProps onPanelFocused and onPanelBlurred (panel: PanelState) to (panel: string) resolved Problems in TS
@rshen91
gridData id changed to i
4d7933f
@rshen91
Resolved ts testing type errors with DashboardPanelUiProps in dashboa…
d1ece70 
…rd_panel.tsx

Updated snapshot for dashboard_panel.test.tsx
@rshen91
Code review from Emma with changes partial PanelState and defining si…
05c96a5 
…ze_x and size_y to numbers

adding comment to clarify panel.panelIndex.toString()
@rshen91
test commit to take in Emmas feedback and show the panel_utils and ty…
d810f74 
…pes.ts modified to show panelIndex string | number

reverting panel_utils and types.ts changes with panelIndex, keeping other changes from Emmas suggestions
@rshen91
to.equal panel_state.ts
f0c4225
@rshen91
Fixed panel_utils and panel_utils test to create updatedPanel object …
1a4d125 
…instead of mutation

revert mutation in panel utils

removed ts-ignore in __tests__/panel_state.ts
@rshen91
clarifying return values in panel_utils
d1d84e4
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@rshen91 rshen91 merged commit 3702ac1 into elastic:master Mar 4, 2019
@rshen91
Copy link
Contributor Author

rshen91 commented Mar 4, 2019

In the panel_utils.ts file, convertPanelDataPre_6_1 and convertPanelDataPre_6_3 mutate the panel instead of creating a new panel object with modifications as needed. This PR did not change this, but should be explored in a future PR. This should be considered to reduce time debugging panels.

@rshen91 rshen91 deleted the rshen-dev-ts branch March 4, 2019 22:24
@rshen91 rshen91 mentioned this pull request Mar 5, 2019
Merged
rshen91 added a commit to rshen91/kibana that referenced this pull request Mar 5, 2019
@rshen91
Typescript dashboard stuff (elastic#27167)
d093d04 
* gridData id changed to i

* to.equal panel_state.ts

* clarifying return values in panel_utils

*fixed imports in dpc

consitency across id -> i for gridData, improvment made in dashboard_panel.tsx

changed DashboardPanelUiProps onPanelFocused and onPanelBlurred (panel: PanelState) to (panel: string) resolved Problems in TS

* Resolved ts testing type errors with DashboardPanelUiProps in dashboard_panel.tsx

Updated snapshot for dashboard_panel.test.tsx

* Code review from Emma with changes partial PanelState and defining size_x and size_y to numbers

adding comment to clarify panel.panelIndex.toString()

* test commit to take in Emmas feedback and show the panel_utils and types.ts modified to show panelIndex string | number

reverting panel_utils and types.ts changes with panelIndex, keeping other changes from Emmas suggestions

* Fixed panel_utils and panel_utils test to create updatedPanel object instead of mutation

Revert mutation in panel utils

removed ts-ignore statements in __tests__/panel_state.ts
rshen91 added a commit that referenced this pull request Mar 5, 2019
@rshen91
Typescript dashboard stuff (#27167) (#32483)
4282f19 
* gridData id changed to i

* to.equal panel_state.ts

* clarifying return values in panel_utils

*fixed imports in dpc

consitency across id -> i for gridData, improvment made in dashboard_panel.tsx

changed DashboardPanelUiProps onPanelFocused and onPanelBlurred (panel: PanelState) to (panel: string) resolved Problems in TS

* Resolved ts testing type errors with DashboardPanelUiProps in dashboard_panel.tsx

Updated snapshot for dashboard_panel.test.tsx

* Code review from Emma with changes partial PanelState and defining size_x and size_y to numbers

adding comment to clarify panel.panelIndex.toString()

* test commit to take in Emmas feedback and show the panel_utils and types.ts modified to show panelIndex string | number

reverting panel_utils and types.ts changes with panelIndex, keeping other changes from Emmas suggestions

* Fixed panel_utils and panel_utils test to create updatedPanel object instead of mutation

Revert mutation in panel utils

removed ts-ignore statements in __tests__/panel_state.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacey-gammon stacey-gammon stacey-gammon left review comments

@emmacunningham emmacunningham emmacunningham left review comments

@mattkime mattkime mattkime approved these changes

Assignees
No one assigned
Labels
Feature:Dashboard Dashboard related features v7.2.0 v8.0.0 WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@rshen91 @elasticmachine @emmacunningham @stacey-gammon @mattkime @rayafratkina