TypeScript cleanup in visualizations plugin #78428

timroes · 2020-09-24T14:12:32Z

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.

~~[ ] 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~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~[ ] This was checked for cross-browser compatibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~

timroes · 2020-09-28T08:09:54Z

@elasticmachine merge upstream

timroes · 2020-09-28T08:12:30Z

@elasticmachine merge upstream

elasticmachine · 2020-09-28T08:14:47Z

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

timroes · 2020-09-28T08:14:58Z

@elasticmachine merge upstream

stratoula · 2020-09-28T08:40:05Z

@elasticmachine merge upstream

stratoula

Code LGTM 💯

timroes · 2020-09-28T08:57:04Z

src/plugins/input_control_vis/public/vis_controller.tsx


 export const createInputControlVisController = (deps: InputControlVisDependencies) => {
  return class InputControlVisController {
    private I18nContext?: I18nStart['Context'];
-    private isLoaded = false;
+    private _isLoaded = false;


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

timroes · 2020-09-28T08:58:48Z

src/plugins/visualizations/public/types.ts

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

-export type VisToExpressionAst = (vis: Vis, params: VisToExpressionAstParams) => string;
+export type VisToExpressionAst = (


ℹ️ 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).

timroes · 2020-09-28T09:00:13Z

src/plugins/visualizations/public/types.ts

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

 export type VisualizationControllerConstructor = new (
  el: HTMLElement,
-  vis: Vis
+  vis: ExprVis


ℹ️ 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.

timroes · 2020-09-28T09:01:48Z

src/plugins/visualizations/public/vis_types/base_vis_type.ts

+  visualization: VisualizationControllerConstructor | undefined;
+}
+
+export type BaseVisTypeOptions = ExpressionBaseVisTypeOptions | VisualizationBaseVisTypeOptions;


ℹ️ 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.

timroes · 2020-09-28T09:02:37Z

src/plugins/visualizations/public/vis_types/types_service.ts

       */
-      createBaseVisualization: (config: any) => {
+      createBaseVisualization: (config: BaseVisTypeOptions): void => {


ℹ️ 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 · 2020-09-28T10:13:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 95c43bb

History

💚 Build #76936 succeeded f4e37cb

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

ppisljar

code LGTM

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

@timestamp

* 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) ...

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

…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>

TypeScript cleanup in visualizations plugin

f4e37cb

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 marked this pull request as ready for review September 28, 2020 08:14

timroes requested a review from a team September 28, 2020 08:14

Merge branch 'master' into refactor/visualizations-ts

95c43bb

stratoula approved these changes Sep 28, 2020

View reviewed changes

timroes commented Sep 28, 2020

View reviewed changes

timroes added the v7.10.0 label Sep 28, 2020

ppisljar approved these changes Sep 28, 2020

View reviewed changes

timroes merged commit a71d069 into elastic:master Sep 28, 2020

timroes deleted the refactor/visualizations-ts branch September 28, 2020 12:33

timroes mentioned this pull request Sep 28, 2020

[7.x] TypeScript cleanup in visualizations plugin (#78428) #78574

Closed

timroes pushed a commit to timroes/kibana that referenced this pull request Sep 28, 2020

TypeScript cleanup in visualizations plugin (elastic#78428)

2d2bdfb

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

sulemanof mentioned this pull request Sep 28, 2020

Fix visualizations types #78619

Merged

7 tasks

timroes pushed a commit to timroes/kibana that referenced this pull request Sep 28, 2020

TypeScript cleanup in visualizations plugin (elastic#78428)

1e117b4

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

timroes mentioned this pull request Sep 28, 2020

[7.x] TypeScript cleanup in visualizations plugin (#78428) | Implement tagcloud renderer (#77910) | Fix types (#78619) #78666

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript cleanup in visualizations plugin #78428

TypeScript cleanup in visualizations plugin #78428

timroes commented Sep 24, 2020 •

edited

Loading

timroes commented Sep 28, 2020

timroes commented Sep 28, 2020

elasticmachine commented Sep 28, 2020

timroes commented Sep 28, 2020

stratoula commented Sep 28, 2020

stratoula left a comment

timroes Sep 28, 2020

timroes Sep 28, 2020

timroes Sep 28, 2020

timroes Sep 28, 2020

timroes Sep 28, 2020

kibanamachine commented Sep 28, 2020

ppisljar left a comment

TypeScript cleanup in visualizations plugin #78428

TypeScript cleanup in visualizations plugin #78428

Conversation

timroes commented Sep 24, 2020 • edited Loading

Summary

Checklist

For maintainers

timroes commented Sep 28, 2020

timroes commented Sep 28, 2020

elasticmachine commented Sep 28, 2020

timroes commented Sep 28, 2020

stratoula commented Sep 28, 2020

stratoula left a comment

Choose a reason for hiding this comment

timroes Sep 28, 2020

Choose a reason for hiding this comment

timroes Sep 28, 2020

Choose a reason for hiding this comment

timroes Sep 28, 2020

Choose a reason for hiding this comment

timroes Sep 28, 2020

Choose a reason for hiding this comment

timroes Sep 28, 2020

Choose a reason for hiding this comment

kibanamachine commented Sep 28, 2020

💚 Build Succeeded

History

ppisljar left a comment

Choose a reason for hiding this comment

timroes commented Sep 24, 2020 •

edited

Loading