elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.73k stars 8.14k forks source link

Observables in the plugin api #13608

Closed kimjoar closed 6 years ago

kimjoar commented 7 years ago

One of the stated goals of the new platform (see https://github.com/elastic/kibana/issues/9675) is that should "not expose [...] dependencies to plugins". As such we need to think about how we expose observables. The Observables proposal is still only stage 1, so we can't rely directly on it yet.

What we could do is expose something that matches that for now, including the essential Symbol.observable (ponyfill), which will enable plugins to use any observables library in their code.

Then, to use a Kibana exposed observable in a plugin

/cc @azasypkin @epixa

epixa commented 7 years ago

I think providing a common to-spec implementation of observable is a great approach. There is risk that the observable spec will change, but the same risk applies with any observable like library, since they all likely want to be compatible with native observables, so they would change as well to be compatible with spec.

kimjoar commented 7 years ago

This might not be as easy as I first thought.

Pulling in a comment from the PR:

For some reason I only thought about the rxjs observables directly exposed. It can actually be somewhat painful to handle them all. Didn't think of that. The problem is what happens when we expose an instance of a class that uses rxjs.

Been thinking about this problem today, and I'm not sure how to best solve this. It's trivial for "top-level values", but it's not easy for observables that exists within created instances.

Two ideas:

I'll let this lay for a couple days while processing this problem.

epixa commented 7 years ago

@kjbekkelund Can you give an example of when it's not easy to use "native" observables?

kimjoar commented 7 years ago

Ah, sorry, I should have done that already.

Methods like getClusterOfType$ here: https://github.com/elastic/kibana/blob/b033a4c22998fbd6f5531d6a4c8c40fe2c9e1082/platform/server/elasticsearch/ElasticsearchService.ts#L66

Here's it's being used in a plugin: https://github.com/elastic/kibana/blob/new-platform/core_plugins/savedObjects/src/SavedObjectsService.ts#L17

(btw, I know the implementation of clusters in that ElasticsearchService is wrong — it's limited to data and admin, but there can of course be more)

For all such methods, we would have to implement a "wrapper" in the api that calls through to the actual method, receives an rxjs observable, transforms to es observable, and returns that to the plugin. (That way we can use rxjs directly internally in the platform, but get ES observables outside)

We might want to be that specific in the api though. Or we might just always return ES observables from that method, and transform to rxjs observables in the platform where they are used.

(Maybe I'm over-complicating things... Hm, I can let this lay for a couple days, then get back to it. Maybe that clears up my thoughts on it.)

epixa commented 7 years ago

I think that helps clear it up for me. I figured we would would be creating facades in front of our internal functions anyway when returning them in a contract, so doing a conversion at that point to a raw observable seems fine enough to me. We can always provide convenience functions to plugins to handle the conversion if we want to help remove boilerplate.

kimjoar commented 7 years ago

Yeah, I think that's the way to go. I'll play around with it a bit more tomorrow to make sure we don't expose any rxjs stuff now, then we can tackle it if we realise that approach won't work at scale.

kimjoar commented 6 years ago

Closed by https://github.com/elastic/kibana/pull/14209