[Vega] Shim new platform #40032

alexwizp · 2019-07-01T14:32:53Z

Summary

Part of #39915

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

src/legacy/core_plugins/vega/public/vega_request_handler.js

src/legacy/core_plugins/vega/public/vega_visualization.js

lukeelmers

I know this is still a draft, so I just tried to answer questions and added a few notes. LMK if there are other questions that come up or when this is ready for a final review.

src/legacy/core_plugins/vega/public/vega_request_handler.js

src/legacy/core_plugins/vega/public/vega_visualization.js

src/legacy/core_plugins/vega/public/vega_view/vega_base_view.js

src/legacy/core_plugins/vega/public/vega_request_handler.js

src/legacy/core_plugins/vega/public/vega_visualization.js

src/legacy/core_plugins/vega/index.ts

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

src/legacy/core_plugins/vega/public/index.ts

src/legacy/core_plugins/vega/index.ts

src/legacy/core_plugins/vega/public/index.ts

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

streamich · 2019-07-05T12:33:38Z

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

+import { VegaSetupPlugin } from './setup';
+
+export class VegaPlugin {
+  public async setup({ data, visualizations, legacy }: VegaSetupPlugin) {


I like this approach where all legacy stuff is injected into the shim instead of imported from ui/....

@elastic/kibana-app-arch Let's discuss it on Tuesday sync.

I would rename it to something like __LEGACY and it would need to be a "plugin", not service. But I somehow like this much more than arbitrarily importing from ui/... from various files within a shim. This way you explicitly see what legacy stuff your plugin still depends on.

Maybe we could have a convention where shims are in <legacy_plugin>/public/shim folder and add a linter rule that does not allow to import anything from ui/... in any of the <legacy_plugin>/public/shim/public/** files.

Yeah that's certainly an option too. To prevent confusion, we should move this discussion over to #39554 and can report back here

streamich · 2019-07-05T12:36:10Z

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

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


If we go with this legacy approach, I guess it should be named "plugin", but does not really matter now.

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

src/legacy/core_plugins/vega/public/vega_request_handler.js

elasticmachine · 2019-07-08T14:53:34Z

Pinging @elastic/kibana-app

lukeelmers

No other major concerns on my end, I think we just owe you some clarification on the conventions we want to be using for shimming. We are discussing this tomorrow and will follow up here with any final items; sorry for all the back and forth!

elasticmachine · 2019-07-09T10:34:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-09T11:22:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T09:16:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-11T10:41:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-12T10:33:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alexwizp · 2019-07-12T15:54:32Z

retest

elasticmachine · 2019-07-12T16:56:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

This makes sense to me! There will be more shimming work to do later in the process (like removing some of the ui/chrome imports), but those don't need to happen right now.

My only other comment would be that we should probably get rid of setup.ts and either moving the contents of that file to legacy.ts, or to the shim directory. This was one of the conventions we decided on recently... having legacy.ts contain (or import) our shims and export the setup/start contracts, rather than having a dedicated setup.ts file.

Otherwise, everything LGTM!

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

elasticmachine · 2019-07-15T09:28:44Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-18T13:50:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

lukeelmers

🎉

alexwizp requested review from lukeelmers and streamich July 1, 2019 14:32

alexwizp commented Jul 1, 2019

View reviewed changes

src/legacy/core_plugins/vega/public/vega_request_handler.js Outdated Show resolved Hide resolved

alexwizp commented Jul 1, 2019

View reviewed changes

src/legacy/core_plugins/vega/public/vega_request_handler.js Outdated Show resolved Hide resolved

alexwizp commented Jul 1, 2019

View reviewed changes

src/legacy/core_plugins/vega/public/vega_visualization.js Outdated Show resolved Hide resolved

lukeelmers reviewed Jul 1, 2019

View reviewed changes

timroes mentioned this pull request Jul 1, 2019

Convert Vega to New Platform #38247

Closed

4 tasks

lukeelmers mentioned this pull request Jul 1, 2019

App Architecture roadmap for New Platform migration #31968

Closed

alexwizp force-pushed the vega branch from 8a24227 to 0797de7 Compare July 4, 2019 07:33

elastic deleted a comment from elasticmachine Jul 4, 2019

alexwizp force-pushed the vega branch from e6ce0bc to ed171fb Compare July 4, 2019 16:25

alexwizp requested a review from lizozom July 5, 2019 07:42

alexwizp force-pushed the vega branch 2 times, most recently from 4027d83 to ee37f4e Compare July 5, 2019 12:36

elastic deleted a comment from elasticmachine Jul 5, 2019

streamich reviewed Jul 5, 2019

View reviewed changes

alexwizp force-pushed the vega branch from fae9994 to 47bd844 Compare July 8, 2019 11:49

elastic deleted a comment from elasticmachine Jul 8, 2019

alexwizp added Feature:Vega Vega visualizations Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.4.0 v8.0.0 labels Jul 8, 2019

lukeelmers added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Jul 8, 2019

lukeelmers reviewed Jul 8, 2019

View reviewed changes

lukeelmers mentioned this pull request Jul 8, 2019

chore: 🤖 forbid import from ui/** #40537

Merged

alexwizp requested a review from lukeelmers July 11, 2019 21:46

lukeelmers reviewed Jul 13, 2019

View reviewed changes

src/legacy/core_plugins/vega/public/plugin.ts Outdated Show resolved Hide resolved

alexwizp force-pushed the vega branch from f6ee44c to f35dd03 Compare July 18, 2019 11:57

alexwizp requested review from nreese, streamich and lukeelmers July 18, 2019 11:58

[Vega] Shim new platform

3c1fe28

alexwizp force-pushed the vega branch from f35dd03 to 3c1fe28 Compare July 18, 2019 12:52

lukeelmers approved these changes Jul 18, 2019

View reviewed changes

alexwizp merged commit 206266d into elastic:master Jul 19, 2019

alexwizp mentioned this pull request Jul 19, 2019

[7.x] [Vega] Shim new platform (#40032) #41562

Merged

alexwizp added a commit to alexwizp/kibana that referenced this pull request Jul 19, 2019

[Vega] Shim new platform (elastic#40032)

bf44098

alexwizp added a commit that referenced this pull request Jul 19, 2019

[Vega] Shim new platform (#40032) (#41562)

d1fad06

lukeelmers mentioned this pull request Jul 22, 2019

[New Platform Migration Phase I]: convert vega plugin to vis_type_vega #39915

Closed

8 tasks

alexwizp deleted the vega branch January 4, 2020 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vega] Shim new platform #40032

[Vega] Shim new platform #40032

alexwizp commented Jul 1, 2019 •

edited

Loading

lukeelmers left a comment

streamich Jul 5, 2019 •

edited

Loading

lukeelmers Jul 8, 2019

streamich Jul 5, 2019

elasticmachine commented Jul 8, 2019

lukeelmers left a comment

elasticmachine commented Jul 9, 2019

elasticmachine commented Jul 9, 2019

elasticmachine commented Jul 10, 2019

elasticmachine commented Jul 11, 2019

elasticmachine commented Jul 12, 2019

alexwizp commented Jul 12, 2019

elasticmachine commented Jul 12, 2019

lukeelmers left a comment

elasticmachine commented Jul 15, 2019

elasticmachine commented Jul 18, 2019

lukeelmers left a comment

[Vega] Shim new platform #40032

[Vega] Shim new platform #40032

Conversation

alexwizp commented Jul 1, 2019 • edited Loading

Summary

Checklist

For maintainers

lukeelmers left a comment

Choose a reason for hiding this comment

streamich Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

lukeelmers Jul 8, 2019

Choose a reason for hiding this comment

streamich Jul 5, 2019

Choose a reason for hiding this comment

elasticmachine commented Jul 8, 2019

lukeelmers left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 9, 2019

💚 Build Succeeded

elasticmachine commented Jul 9, 2019

💚 Build Succeeded

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

elasticmachine commented Jul 12, 2019

💔 Build Failed

alexwizp commented Jul 12, 2019

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 18, 2019

💚 Build Succeeded

lukeelmers left a comment

Choose a reason for hiding this comment

alexwizp commented Jul 1, 2019 •

edited

Loading

streamich Jul 5, 2019 •

edited

Loading