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

[New platform]Expose NP core services to the legacy UI #32480

Merged
merged 4 commits into from
Mar 7, 2019

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Mar 5, 2019

Summary

#32395
Expose NP core services to the legacy UI platform. There are no plugins to include yet.

Checklist

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

For maintainers

Dev Docs

Expose NP core services to the legacy UI

Kibana is in process of stabilising of API. The client side core services are quite different from existing ones and were heavily refactored. If you want to play around with experimental API use

import { getNewPlatform } from 'ui/new_platform';
const newPlatform = getNewPlatform();

where newPlatform exposes:

Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

Good stuff. One thing though: while stop doesn’t seem necessary, I think we will be adding a separate init lifecycle event alongside start, and both of those contracts will be relevant to legacy plugins.

This isn’t 100% final yet, but perhaps it is worth identifying these apis as “start” so folks don’t need to update their code as much when init happens.

throw new Error('New platform api was already initialized');
}

runtimeContext.core = Object.freeze(core);
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 found several deepFreeze implementation in our code base. Should we have a dedicated package/wheterver for this?

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov changed the title Expose NP core services to the legacy UI [New platform]Expose NP core services to the legacy UI Mar 5, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov
Copy link
Contributor Author

mshustov commented Mar 5, 2019

@epixa should we add integration tests here? We can test only getter logic, which is not really useful.

@epixa
Copy link
Contributor

epixa commented Mar 5, 2019

@restrry I don't think we should integration test the ui module shims

@mshustov mshustov added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.2.0 labels Mar 6, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@mshustov mshustov requested a review from a team March 6, 2019 10:27
@mshustov mshustov marked this pull request as ready for review March 6, 2019 10:27
Copy link
Contributor

@epixa epixa left a comment

Choose a reason for hiding this comment

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

LGTM

I added a couple comments/nits. I'm curious why we need the duplication with the RuntimeContext, which I commented on. It would be great if we could limit that kind of duplication, but it's non-critical.

chrome: ChromeStart;
}

interface RuntimeContext {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is the whole reason we need this interface because of the conditional existence of start.core?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. as an alternative we can cast type right in place.

const runtimeContext = {
  start: {
    core: null as StartCore | null,

src/legacy/ui/public/new_platform/new_platform.ts Outdated Show resolved Hide resolved
src/legacy/ui/public/new_platform/new_platform.ts Outdated Show resolved Hide resolved
src/legacy/ui/public/new_platform/new_platform.ts Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mshustov mshustov merged commit d98538b into elastic:master Mar 7, 2019
@mshustov mshustov deleted the issue-32395-client-side branch March 7, 2019 15:08
mshustov added a commit to mshustov/kibana that referenced this pull request Mar 7, 2019
* Expose NP core services to the legacy UI

* declare start/stop explicitly

* simplify typings + renaming
mshustov added a commit that referenced this pull request Mar 7, 2019
* Expose NP core services to the legacy UI

* declare start/stop explicitly

* simplify typings + renaming
@mshustov mshustov added backported and removed review labels Mar 8, 2019
@epixa epixa added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label May 7, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants