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

Data plugin: Expose setup apis also on start #44903

Merged
merged 3 commits into from Sep 9, 2019

Conversation

flash1293
Copy link
Contributor

Summary

As discussed offline, expose all of the setup apis of the data plugin also in start phase.
Inspired by src/legacy/core_plugins/embeddable_api/public/np_ready/public/plugin.ts.

Checklist

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

For maintainers

@flash1293 flash1293 added v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Sep 5, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@flash1293 flash1293 changed the title Expose setup apis also on start Data plugin: Expose setup apis also on start Sep 5, 2019
expressions: this.expressions.start({ inspector: plugins.inspector }),
...this.setupApi!,
expressions: {
...this.setupApi!,
Copy link
Contributor

@lizozom lizozom Sep 5, 2019

Choose a reason for hiding this comment

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

Why are you destructuring setupApi into expressions?
Looking at the API, I don't think you need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, I meant to spread ...this.setupApi!.expressions

Copy link
Contributor

@lizozom lizozom Sep 5, 2019

Choose a reason for hiding this comment

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

I don't think you should.
They have different APIs ATM.
If you need the same functionality in start, that should be a change in the expressions sub-service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't need it, just thought I put it in there for completeness sake. I will remove it for now.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293
Copy link
Contributor Author

@lizozom You can check the PR again.

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.

LGTM

@flash1293 flash1293 merged commit 63868a1 into elastic:master Sep 9, 2019
@flash1293 flash1293 deleted the data/expose-setup-on-start branch September 9, 2019 13:08
flash1293 added a commit to flash1293/kibana that referenced this pull request Sep 9, 2019
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.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants