Skip to content

Data plugin: Expose setup apis also on start#44903

Merged
flash1293 merged 3 commits intoelastic:masterfrom
flash1293:data/expose-setup-on-start
Sep 9, 2019
Merged

Data plugin: Expose setup apis also on start#44903
flash1293 merged 3 commits intoelastic:masterfrom
flash1293:data/expose-setup-on-start

Conversation

@flash1293
Copy link
Copy Markdown
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
@flash1293 flash1293 requested a review from lizozom September 5, 2019 15:50
@elasticmachine
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@flash1293
Copy link
Copy Markdown
Contributor Author

@lizozom You can check the PR again.

Copy link
Copy Markdown
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.

3 participants