Improve visualization typings #79128

timroes · 2020-10-01T14:36:39Z

Summary

This PR improves the typings of the visualizations plugin and types the VisType interface explicitly and remove the any index signature of it. I still have some types in there (like the editor) I want to improve, but since they touch multiple plugins I want to improve them in separate PRs after this.

For more details on the typings check the inline comments.

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-10-01T14:38:58Z

src/plugins/tile_map/public/tile_map_type.js

@@ -55,7 +55,6 @@ export function createTileMapTypeDefinition(dependencies) {
        wms: uiSettings.get('visualization:tileMap:WMSdefaults'),
      },
    },
-    requiresPartialRows: true,


ℹ️ This flag was never passed through the BaseVisType, so it seems this feature is no longer needed for anything and I removed the flag completely.

timroes · 2020-10-01T14:42:22Z

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

-    this.editorConfig = _.defaultsDeep({}, opts.editorConfig, { collections: {} });
-    this.options = _.defaultsDeep({}, opts.options, defaultOptions);
-    this.stage = opts.stage || 'production';
-    this.isExperimental = opts.stage === 'experimental';


ℹ️ The isExperimental flag was only used in one place. I just inlined the check against stage there, so keep the VisType interface a bit smaller.

timroes · 2020-10-02T13:28:35Z

src/plugins/visualizations/public/wizard/new_vis_modal.test.tsx

@@ -107,6 +118,30 @@ describe('NewVisModal', () => {
    expect(wrapper.find('[data-test-subj="visType-vis"]').exists()).toBe(true);
  });

+  it('should sort promoted visualizations first', () => {


ℹ️ I've added this test mainly, because during work on this I broke it, and the only thing that catched the broken ordering was a snapshot test, where it was not quit clear in the beginning that it was the ordering that was broken. So I added a more explicit test on this.

timroes · 2020-10-02T17:04:21Z

src/plugins/visualizations/public/wizard/type_selection/type_selection.tsx

    } else {
      const q = query.toLowerCase();
      entries = allTypes.map((type) => {
        const matchesQuery =
          type.name.toLowerCase().includes(q) ||
          type.title.toLowerCase().includes(q) ||
          (typeof type.description === 'string' && type.description.toLowerCase().includes(q));
-        return { ...type, highlighted: matchesQuery };


ℹ️ The main problem here was that we were spreading the type here and thus losing the actual BaseVisTypeClass that has getters on it. I changed this structure slightly, so that we now just assign the type as a reference into each VisTypeListEntry. All the further changes in this file are related to this minor change.

timroes · 2020-10-02T17:05:27Z

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

-      if (this.unregisteredHiddenTypes.includes(visDefinition.name)) {
-        visDefinition.hidden = true;
-      }
+  private registerVisualization<TVisParam>(visDefinition: VisType<TVisParam>) {


ℹ️ Just pulling this into an actual class method, instead of defining it as a closure, since it did not needed any access to anything that closure would have had.

timroes · 2020-10-02T17:05:54Z

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

+  /**
+   * The editor that should be used to edit visualizations of this type.
+   */
+  readonly editor?: any;


ℹ️ I still plan to type this properly, but I need to touch a bit more plugins for this, thus I'd prefer doing it in a separate PR.

timroes · 2020-10-02T17:06:48Z

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

+  hierarchicalData: false, // we should get rid of this i guess ?
+};
+
+export class BaseVisType<TVisParams = VisParams> implements VisType<TVisParams> {


ℹ️ Those now properly implement the interface and thus you also don't need to type the properties here again, since it can pull it automatically from the interface.

timroes · 2020-10-05T09:15:42Z

@elasticmachine merge upstream

elasticmachine · 2020-10-05T09:19:42Z

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

stratoula

Code LGTM, also tested it locally, can't find any regressions. 🍪

flash1293

Didn't find any issues - great start for cleaning up the types in here!

flash1293 · 2020-10-06T09:56:31Z

src/plugins/visualizations/public/legacy/build_pipeline.ts

@@ -155,7 +158,8 @@ export const getSchemas = (vis: Vis, { timeRange, timefilter }: BuildPipelinePar
      }
    }
    if (schemaName === 'split') {
-      schemaName = `split_${vis.params.row ? 'row' : 'column'}`;
+      // TODO: We should check if there's a better way then casting to `any` here
+      schemaName = `split_${(vis.params as any).row ? 'row' : 'column'}`;


We could restrict TVisParams a little further (<TVisParams extends { row?: boolean }>), but this file is not properly typed in a lot of places so I wouldn't bother improving it. We should be able to remove it and move the logic into the individual visualizations where they can be typed explicitly anyway.

kibanamachine · 2020-10-06T10:31:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 37261b7

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`visualizations`	134	135	+1

async chunks size

id	before	after	diff
`visDefaultEditor`	409.7KB	409.7KB	-37.0B
`visualize`	271.4KB	271.4KB	+8.0B
total			-29.0B

page load bundle size

id	before	after	diff
`tileMap`	49.1KB	49.2KB	+87.0B
`visTypeMetric`	27.2KB	27.1KB	-30.0B
`visTypeTable`	77.8KB	78.1KB	+228.0B
`visTypeTagcloud`	22.1KB	22.0KB	-30.0B
`visualizations`	273.3KB	273.5KB	+170.0B
total			+425.0B

History

💚 Build #79586 succeeded 2c03b43
💔 Build #79520 failed deb5450
💚 Build #79509 succeeded feabd17
💚 Build #79151 succeeded 5c2f793
💔 Build #78771 failed 7048212

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

* Improve visualization typings * Fix vis selection dialog * Fix broken getInfoMessage type Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* Improve visualization typings * Fix vis selection dialog * Fix broken getInfoMessage type Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Improve visualization typings

7048212

timroes commented Oct 1, 2020

View reviewed changes

Fix vis selection dialog

5c2f793

timroes commented Oct 2, 2020

View reviewed changes

Merge branch 'master' into chore/improve-visualization-typings

feabd17

timroes added 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 labels Oct 5, 2020

timroes requested review from stratoula and flash1293 October 5, 2020 09:19

timroes marked this pull request as ready for review October 5, 2020 09:19

timroes requested a review from a team October 5, 2020 09:19

timroes changed the title ~~Improve visualization typings [WIP]~~ Improve visualization typings Oct 5, 2020

Tim Roes added 3 commits October 5, 2020 12:09

Merge branch 'master' into chore/improve-visualization-typings

deb5450

Fix broken getInfoMessage type

2c03b43

Merge branch 'master' into chore/improve-visualization-typings

37261b7

stratoula approved these changes Oct 6, 2020

View reviewed changes

flash1293 approved these changes Oct 6, 2020

View reviewed changes

timroes merged commit b8f4ea1 into elastic:master Oct 6, 2020

timroes deleted the chore/improve-visualization-typings branch October 6, 2020 11:05

timroes mentioned this pull request Oct 6, 2020

[7.x] Improve visualization typings (#79128) #79636

Merged

timroes added the hacktoberfest-accepted https://hacktoberfest.digitalocean.com/hacktoberfest-update label Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve visualization typings #79128

Improve visualization typings #79128

timroes commented Oct 1, 2020 •

edited

Loading

timroes Oct 1, 2020

timroes Oct 1, 2020

timroes Oct 2, 2020

timroes Oct 2, 2020

timroes Oct 2, 2020

timroes Oct 2, 2020

timroes Oct 2, 2020

stratoula Oct 6, 2020

timroes commented Oct 5, 2020

elasticmachine commented Oct 5, 2020

stratoula left a comment

flash1293 left a comment

flash1293 Oct 6, 2020

kibanamachine commented Oct 6, 2020

Improve visualization typings #79128

Improve visualization typings #79128

Conversation

timroes commented Oct 1, 2020 • edited Loading

Summary

Checklist

For maintainers

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes commented Oct 5, 2020

elasticmachine commented Oct 5, 2020

stratoula left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 6, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

page load bundle size

History

timroes commented Oct 1, 2020 •

edited

Loading