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

Use embeddable v2 - fix regression of #39126 #41221

Merged
merged 2 commits into from
Jul 18, 2019

Conversation

alexwizp
Copy link
Contributor

Summary

Fix regression of #39126

Constructors cannot be async, but static methods can. It’s pretty easy to have a static creation method, making the type its own factory

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@alexwizp alexwizp added release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0 Team:AppArch labels Jul 16, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@alexwizp alexwizp added the Feature:Embedding Embedding content via iFrame label Jul 16, 2019
@elastic elastic deleted a comment from elasticmachine Jul 16, 2019
@alexwizp
Copy link
Contributor Author

retest

@elastic elastic deleted a comment from elasticmachine Jul 16, 2019
@elastic elastic deleted a comment from elasticmachine Jul 16, 2019
@alexwizp
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

Code review only, LGTM on green ci. Thanks for fixing!

@elastic elastic deleted a comment from elasticmachine Jul 17, 2019
@elastic elastic deleted a comment from elasticmachine Jul 17, 2019
@elastic elastic deleted a comment from elasticmachine Jul 17, 2019
@elastic elastic deleted a comment from elasticmachine Jul 17, 2019
@elastic elastic deleted a comment from elasticmachine Jul 17, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@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 like this approach to dealing with the async registry issue because:

  • We don't have to wait on the injector before loading up our shimmed plugins
  • It doesn't require any hackery within the plugins themselves
  • We know the registry implementation will be changing in the near term anyway.

Let's just get a final review from @streamich before merging, otherwise this LGTM!

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

LGTM

src/legacy/ui/public/registry/_registry.js Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alexwizp alexwizp merged commit c03698f into elastic:master Jul 18, 2019
alexwizp added a commit to alexwizp/kibana that referenced this pull request Jul 18, 2019
* Use embeddable v2 - fix regression of elastic#39126

* fix PR comment
alexwizp added a commit that referenced this pull request Jul 18, 2019
* Use embeddable v2 - fix regression of #39126

* fix PR comment
@alexwizp alexwizp deleted the 39126-fix branch January 4, 2020 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants