Skip to content

[Vega] Shim new platform#40032

Merged
alexwizp merged 1 commit intoelastic:masterfrom
alexwizp:vega
Jul 19, 2019
Merged

[Vega] Shim new platform#40032
alexwizp merged 1 commit intoelastic:masterfrom
alexwizp:vega

Conversation

@alexwizp
Copy link
Copy Markdown
Contributor

@alexwizp alexwizp commented Jul 1, 2019 •
edited
Loading

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.

For maintainers

@alexwizp alexwizp requested review from lukeelmers and streamich July 1, 2019 14:32
alexwizp
alexwizp commented Jul 1, 2019
View reviewed changes
alexwizp
alexwizp commented Jul 1, 2019
View reviewed changes
alexwizp
alexwizp commented Jul 1, 2019
View reviewed changes
lukeelmers
lukeelmers reviewed Jul 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

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.

@timroes timroes mentioned this pull request Jul 1, 2019
Closed
4 tasks
@lukeelmers lukeelmers mentioned this pull request Jul 1, 2019
Closed
@elastic elastic deleted a comment from elasticmachine Jul 4, 2019
@elastic elastic deleted a comment from elasticmachine Jul 4, 2019
@alexwizp alexwizp requested a review from lizozom July 5, 2019 07:42
@alexwizp alexwizp force-pushed the vega branch 2 times, most recently from 4027d83 to ee37f4e Compare July 5, 2019 12:36
@elastic elastic deleted a comment from elasticmachine Jul 5, 2019
streamich
streamich reviewed Jul 5, 2019
View reviewed changes
src/legacy/core_plugins/vega/public/plugin.ts Outdated
Copy link
Copy Markdown
Contributor

@streamich streamich Jul 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers Jul 8, 2019

Choose a reason for hiding this comment

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

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

src/legacy/core_plugins/vega/public/setup.ts Outdated
Copy link
Copy Markdown
Contributor

@streamich streamich Jul 5, 2019

Choose a reason for hiding this comment

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

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

@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@elastic elastic deleted a comment from elasticmachine Jul 8, 2019
@alexwizp alexwizp added Feature:Vega Vega visualizations Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.4.0 v8.0.0 labels Jul 8, 2019
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 8, 2019

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 8, 2019

💚 Build Succeeded

@lukeelmers lukeelmers added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Jul 8, 2019
lukeelmers
lukeelmers reviewed Jul 8, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

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!

@lukeelmers lukeelmers mentioned this pull request Jul 8, 2019
Merged
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 9, 2019

💚 Build Succeeded

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 9, 2019

💚 Build Succeeded

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

@alexwizp alexwizp requested a review from lukeelmers July 11, 2019 21:46
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 12, 2019

💔 Build Failed

@alexwizp
Copy link
Copy Markdown
Contributor Author

alexwizp commented Jul 12, 2019

retest

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 12, 2019

💚 Build Succeeded

lukeelmers
lukeelmers reviewed Jul 13, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

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!

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 15, 2019

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jul 18, 2019

💚 Build Succeeded

lukeelmers
lukeelmers approved these changes Jul 18, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

🎉

@alexwizp alexwizp merged commit 206266d into elastic:master Jul 19, 2019
@alexwizp alexwizp mentioned this pull request Jul 19, 2019
Merged
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jul 19, 2019
@alexwizp
[Vega] Shim new platform (elastic#40032)
bf44098
alexwizp added a commit that referenced this pull request Jul 19, 2019
@alexwizp
[Vega] Shim new platform (#40032) (#41562)
d1fad06
@lukeelmers lukeelmers mentioned this pull request Jul 22, 2019
Closed
8 tasks
@alexwizp alexwizp deleted the vega branch January 4, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukeelmers lukeelmers lukeelmers approved these changes

@lizozom lizozom Awaiting requested review from lizozom

@nreese nreese Awaiting requested review from nreese

@streamich streamich Awaiting requested review from streamich

Assignees

@alexwizp alexwizp

Labels

Feature:New Platform Feature:Vega Vega visualizations release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.4.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexwizp @elasticmachine @lukeelmers @streamich