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

TypeScript cleanup in visualizations plugin #78428

Merged
merged 2 commits into from
Sep 28, 2020

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Sep 24, 2020

Summary

This PR cleans up some broken (or not ideal) TypeScript types. See the inline comments for more details.

This PR should not change any functionality.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@timroes timroes added chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Sep 24, 2020
@timroes
Copy link
Contributor Author

timroes commented Sep 28, 2020

@elasticmachine merge upstream

1 similar comment
@timroes
Copy link
Contributor Author

timroes commented Sep 28, 2020

@elasticmachine merge upstream

@timroes timroes marked this pull request as ready for review September 28, 2020 08:14
@timroes timroes requested a review from a team September 28, 2020 08:14
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@timroes
Copy link
Contributor Author

timroes commented Sep 28, 2020

@elasticmachine merge upstream

1 similar comment
@stratoula
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code LGTM 💯


export const createInputControlVisController = (deps: InputControlVisDependencies) => {
return class InputControlVisController {
private I18nContext?: I18nStart['Context'];
private isLoaded = false;
private _isLoaded = 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.

ℹ️ Since the type is propertly passed through now, this is colling with Visualizatins isLoaded() function, so I rename this for now.

@@ -71,4 +73,7 @@ export interface VisToExpressionAstParams {
abortSignal?: AbortSignal;
}

export type VisToExpressionAst = (vis: Vis, params: VisToExpressionAstParams) => string;
export type VisToExpressionAst = (
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 completely wrongly typed :-) This function was never supposed to return a string. It's used in build_pipeline and the result is passed in a function expecting an ExpressionAst (also all implementations currently return that).

@@ -35,7 +37,7 @@ export interface VisualizationController {

export type VisualizationControllerConstructor = new (
el: HTMLElement,
vis: Vis
vis: ExprVis
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 seems there is currently two very similar Vis classes around ExprVis and Vis and they've got mixed up in a couple of places. ExprVis is the instance used in the renderer when intatiating the visualizations, while Vis is the one used by the editor instantiated at a very high level.

visualization: VisualizationControllerConstructor | undefined;
}

export type BaseVisTypeOptions = ExpressionBaseVisTypeOptions | VisualizationBaseVisTypeOptions;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ Properly splitted this up into two types. It either needs to have a visualization or a toExpressionAst, but not both. Technically this COULD still be used that way (since the toExpressionAst could return an expression containing the visualization function, that would then go and look for visualization), but we're not using it this way, since toExpressionAst is used for the functions using their own renderer, so I rather wanted this state to reflect the refactoring we want to end up with, not what technically theoretically would be possible.

*/
createBaseVisualization: (config: any) => {
createBaseVisualization: (config: BaseVisTypeOptions): void => {
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 the most important part of this PR, since it finally puts the proper typings for the visualization registry into the public API we expose.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

code LGTM

@timroes timroes merged commit a71d069 into elastic:master Sep 28, 2020
@timroes timroes deleted the refactor/visualizations-ts branch September 28, 2020 12:33
timroes pushed a commit to timroes/kibana that referenced this pull request Sep 28, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Sep 28, 2020
* master: (226 commits)
  [Enterprise Search] Added Logic for the Credentials View (elastic#77626)
  [CSM] Js errors (elastic#77919)
  Add the @kbn/apm-config-loader package (elastic#77855)
  [Security Solution] Refactor useSelector (elastic#75297)
  Implement tagcloud renderer (elastic#77910)
  [APM] Alerting: Add global option to create all alert types (elastic#78151)
  [Ingest pipelines] Upload indexed document to test a pipeline (elastic#77939)
  TypeScript cleanup in visualizations plugin (elastic#78428)
  Lazy load metric & mardown visualizations (elastic#78391)
  [Detections][EQL] EQL rule execution in detection engine (elastic#77419)
  Update tutorial-full-experience.asciidoc (elastic#75836)
  Update tutorial-define-index.asciidoc (elastic#75754)
  Add support for runtime field types to mappings editor. (elastic#77420)
  [Monitoring] Usage collection (elastic#75878)
  [Docs][Actions] Add docs for Jira and IBM Resilient (elastic#78316)
  [Security Solution][Resolver] Update @timestamp formatting (elastic#78166)
  [Security Solution] Fix app layout (elastic#76668)
  [Security Solution][Resolver] 2 new functions to DAL (elastic#78477)
  Adds new elasticsearch client to telemetry plugin (elastic#78046)
  skip flaky suite (elastic#78512) (elastic#78511) (elastic#78510) (elastic#78509) (elastic#78508) (elastic#78507) (elastic#78506) (elastic#78505) (elastic#78504) (elastic#78503) (elastic#78502) (elastic#78501) (elastic#78500)
  ...
@sulemanof sulemanof mentioned this pull request Sep 28, 2020
7 tasks
timroes pushed a commit to timroes/kibana that referenced this pull request Sep 28, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
timroes pushed a commit that referenced this pull request Sep 29, 2020
…t tagcloud renderer (#77910) | Fix types (#78619) (#78666)

* TypeScript cleanup in visualizations plugin (#78428)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Implement tagcloud renderer (#77910)

* Implement toExpressionAst for tagcloud

* Implement tagcloud vis renderer

* Use resize observer

* Use common no data message

* Update build_pipeline.test

* Update tag cloud tests

* Revert "Use common no data message"

This reverts commit fddf019.

* Update interpreter functional tests

* Add tests for toExpressionAst fn

* Use throttled chart update

* Update renderer

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* Fix types (#78619)

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Daniil Suleiman <31325372+sulemanof@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants