Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Embeddable API cleanup #60207

Merged

Conversation

stacey-gammon
Copy link
Contributor

@stacey-gammon stacey-gammon commented Mar 14, 2020

  • Remove registerEmbeddableFactory from start contract
    • Make isEditable async as that is based off capabilities which is retrieved from CoreStart, so needs to be available async.
  • Remove deprecated GetEmbeddableFactory/ies types
  • Remove the createApi semantics, go with a simpler approach.

@stacey-gammon stacey-gammon requested a review from a team as a code owner March 14, 2020 21:29
@stacey-gammon stacey-gammon requested a review from a team March 14, 2020 21:29
@stacey-gammon stacey-gammon requested a review from a team as a code owner March 14, 2020 21:29
Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

Reviewed changes include

  • Renaming some interfaces (IEmbeddableStart --> EmbeddableStart)
  • Using EmbeddableStart['getEmbeddableFactory'] instead of GetEmbeddableFactory
  • Making isEditable async
  • Using embeddable setup contract instead of start
  • Defining dashboard plugin's start dependecies

LGTM once green

const getStartServices = async () => {
const [coreStart, deps] = await core.getStartServices();

const useHideChrome = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this is going to be re-used in visualize and discover.
Maybe we want to put this in kibana_react?

@@ -27,17 +27,19 @@ import { Embeddable } from './embeddable';
import { SavedObjectIndexStore, DOC_TYPE } from '../../persistence';
import { getEditPath } from '../../../../../../plugins/lens/common';

interface StartServices {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not relevant for this PR, but this object is a great example of how inconsistent our naming is
xxxContract,xxxType,xxxSetup and a type with no suffix at all - in the same object :)
I guess we should choose and stick to one or two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree! I called this startServices based off the naming of core.getStartServices().

@lizozom
Copy link
Contributor

lizozom commented Mar 15, 2020

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lizozom lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Mar 15, 2020
@stacey-gammon stacey-gammon force-pushed the 2020-03-13-embed-interface-cleanup branch from 52686f1 to b81e54f Compare March 16, 2020 13:06
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@stacey-gammon
Copy link
Contributor Author

@oatkiller - if you could take a peek, just made a change to the embeddable api so your code got touched.

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

Discover and Lens changes LGTM. Did not test

Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Thanks!

@stacey-gammon stacey-gammon merged commit dccfa59 into elastic:master Mar 16, 2020
stacey-gammon added a commit to stacey-gammon/kibana that referenced this pull request Mar 16, 2020
* wip

* Remove test in legacy functional plugin
stacey-gammon added a commit that referenced this pull request Mar 17, 2020
* wip

* Remove test in legacy functional plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants