-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Data plugin: Expose setup apis also on start #44903
Conversation
Pinging @elastic/kibana-app-arch |
expressions: this.expressions.start({ inspector: plugins.inspector }), | ||
...this.setupApi!, | ||
expressions: { | ||
...this.setupApi!, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
💚 Build Succeeded |
💚 Build Succeeded |
@lizozom You can check the PR again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
As discussed offline, expose all of the
setup
apis of thedata
plugin also in start phase.Inspired by
src/legacy/core_plugins/embeddable_api/public/np_ready/public/plugin.ts
.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers