[Table Vis] Shim new platform #40732

alexwizp · 2019-07-10T11:32:00Z

Summary

Part of #38245

1 Step of migrating to new Platform

What was done in this PR:

Have a new platform-style plugin definition in typescript, with a setup method returning the public contract (see data plugin as an example). This needs to be exported from the top-level /server and/or /public directory, e.g. export foo = new Plugin.setup()
Be broken into logical "services", each with a service definition class (see data plugin as an example)
Register the visualization via the "visualizations" plugin's types service.
Have all static exports done from the top level plugin definition

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

elasticmachine · 2019-07-10T13:59:18Z

Pinging @elastic/kibana-app

elasticmachine · 2019-07-10T15:56:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T18:59:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lizozom · 2019-07-10T19:21:36Z

src/legacy/core_plugins/table_vis/public/setup.ts

+export const plugins: TablePluginSetupDependencies = {
+  data: {
+    expressions: {
+      registerFunction: (fn: any) => functionsRegistry.register(fn),


We're copying this function across all plugin defenitions.
Lets add it to data.expressions ASAP @ppisljar

lizozom · 2019-07-10T19:27:53Z

src/legacy/core_plugins/table_vis/public/paginated_table/paginated_table.js

 import paginatedTableTemplate from './paginated_table.html';

-import 'ui/directives/paginate';
+export function PaginatedTable($filter) {


I like the idea of separating the component from the directive.
This way we can refactor these in place, and remove the directives later.
Think this is a good practice!

lizozom · 2019-07-10T20:23:51Z

src/legacy/core_plugins/table_vis/public/setup.ts

+
+  // Temporary solution
+  // It will be removed when all dependent services are migrated to the new platform.
+  __LEGACY: new LegacyDependenciesPlugin(),


@lukeelmers @streamich I like how this concept applies to the visualizations and solves the async deps problem.
I think you should have demoed this in our meeting!

I wonder how can this be applied to plugins that make more intensive use of Angular dependencies.

lizozom · 2019-07-10T20:29:34Z

src/legacy/core_plugins/table_vis/public/shim/legacy_dependencies_plugin.ts

+    const Private = $injector.get('Private');
+
+    return {
+      createAngularVisualization: VisFactoryProvider(Private).createAngularVisualization,


I like that you didn't end up exposing raw Angular deps here.
I wonder if this is the case for all visualizations.
But here it works great @streamich

elasticmachine · 2019-07-11T10:22:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-12T10:34:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

Same comment about setup.ts -> legacy.ts as I added to the TSVB & Vega PRs. Otherwise LGTM!

elasticmachine · 2019-07-15T10:21:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

# Conflicts: # src/legacy/core_plugins/table_vis/public/agg_table/agg_table.js # src/legacy/core_plugins/table_vis/public/agg_table/agg_table_group.js # src/legacy/core_plugins/table_vis/public/paginated_table/paginated_table.js # src/legacy/core_plugins/table_vis/public/table_vis_params.js

elasticmachine · 2019-07-22T10:46:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

myasonik

Ran through this manually locally confirming everything seems to work 👍 LGTM

myasonik · 2019-07-22T11:44:55Z

src/legacy/core_plugins/table_vis/public/shim/table_vis_legacy_module.ts

+    .controller('KbnTableVisController', TableVisController)
+    .directive('tableVisParams', TableVisParams);
+
+  // todo: not sure that "kibana" is a right module for that directives


Not sure if this was meant to be a todo before checkin or not but seems like something to resolve before merging

You might be able to get away with using kibana/table_vis for everything since all of these directives are specific to the table vis at this point i think.

@lizozom do you have any insight on this?

@lukeelmers @lizozom @nreese checked it again and looks like we can move all into one module without any risk. PR was updated

elasticmachine · 2019-07-23T11:45:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers · 2019-07-23T22:24:07Z

src/legacy/core_plugins/table_vis/public/plugin.ts

@@ -41,8 +41,7 @@ export interface TableVisualizationDependencies extends LegacyDependenciesPlugin

 /** @internal */
 export interface TablePluginSetupDependencies {
-  // TODO: Remove `any` as functionsRegistry will be added to the DataSetup.
-  data: DataSetup | any;
+  data: ReturnType<DataPublicPlugin['setup']>;


👍 As discussed we'll update this later when we sort out the discrepancy between the src/plugins/data and src/legacy/core_plugins/data interfaces

* [Table Vis] Shim new platform * fix mocha tests * fix mocha tests * fix _agg_config test * cleanup * remove setup.ts * Set the correct type for the "Data" dependency

alexwizp requested review from lukeelmers, lizozom and streamich July 10, 2019 11:32

[Table Vis] Shim new platform

8be116b

alexwizp force-pushed the table_vis branch from 48b1876 to 8be116b Compare July 10, 2019 11:37

fix mocha tests

57959e2

elastic deleted a comment from elasticmachine Jul 10, 2019

alexwizp marked this pull request as ready for review July 10, 2019 13:58

alexwizp added Feature:New Platform Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0 labels Jul 10, 2019

alexwizp self-assigned this Jul 10, 2019

alexwizp added the release_note:skip Skip the PR/issue when compiling release notes label Jul 10, 2019

fix mocha tests

ca6c294

elastic deleted a comment from elasticmachine Jul 10, 2019

fix _agg_config test

6fbe8b1

lizozom reviewed Jul 10, 2019

View reviewed changes

alexwizp added 2 commits July 11, 2019 11:19

Merge remote-tracking branch 'upstream/master'

c98999a

cleanup

a6059f2

alexwizp added the Feature:Data Table Data table visualization feature label Jul 11, 2019

Merge branch 'master' into table_vis

be53c39

lukeelmers reviewed Jul 13, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into table_vis

7d08fda

lukeelmers mentioned this pull request Jul 16, 2019

App Architecture roadmap for New Platform migration #31968

Closed

alexwizp added 2 commits July 22, 2019 12:06

remove setup.ts

b1c3ecc

alexwizp requested a review from myasonik July 22, 2019 10:28

alexwizp requested a review from lizozom July 22, 2019 11:39

myasonik approved these changes Jul 22, 2019

View reviewed changes

alexwizp requested review from nreese and lukeelmers July 22, 2019 12:04

alexwizp added 2 commits July 23, 2019 13:28

Merge remote-tracking branch 'upstream/master' into table_vis

b56b098

Set the correct type for the "Data" dependency

f392744

lukeelmers approved these changes Jul 23, 2019

View reviewed changes

alexwizp merged commit 1ba5a62 into elastic:master Jul 24, 2019

alexwizp mentioned this pull request Jul 24, 2019

[7.x] [Table Vis] Shim new platform (#40732) #41864

Merged

alexwizp mentioned this pull request Jul 30, 2019

[New Platform Migration Phase I]: convert table vis plugin to vis_type_table #41248

Closed

8 tasks

alexwizp deleted the table_vis branch January 4, 2020 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Table Vis] Shim new platform #40732

[Table Vis] Shim new platform #40732

alexwizp commented Jul 10, 2019 •

edited

Loading

elasticmachine commented Jul 10, 2019

elasticmachine commented Jul 10, 2019

elasticmachine commented Jul 10, 2019

lizozom Jul 10, 2019

lizozom Jul 10, 2019

lizozom Jul 10, 2019

lizozom Jul 10, 2019

elasticmachine commented Jul 11, 2019

elasticmachine commented Jul 12, 2019

lukeelmers left a comment

elasticmachine commented Jul 15, 2019

elasticmachine commented Jul 22, 2019

myasonik left a comment

myasonik Jul 22, 2019

lukeelmers Jul 23, 2019

alexwizp Jul 23, 2019

elasticmachine commented Jul 23, 2019

lukeelmers Jul 23, 2019

[Table Vis] Shim new platform #40732

[Table Vis] Shim new platform #40732

Conversation

alexwizp commented Jul 10, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 10, 2019

elasticmachine commented Jul 10, 2019

💔 Build Failed

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

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

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

elasticmachine commented Jul 12, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 15, 2019

💔 Build Failed

elasticmachine commented Jul 22, 2019

💚 Build Succeeded

myasonik 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

elasticmachine commented Jul 23, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

alexwizp commented Jul 10, 2019 •

edited

Loading