[Discuss]: Providing plugin's own start contract from core.getStartServices #61106
Comments
lukeelmers
added
discuss
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
|
One more thing to consider - createGetterSetter proves very effective when you have complex ui components like search bar. If we want to propagate all the required services from the top level, we would have to add them to all of our props. |
|
I understand the need and I personally don't have any objection on implementing that. Let's see what the others have to say about it.
Proposed implementation SGTM |
|
The other big downside to using the getter/setter pattern is that it looks very easy for a new dev to accidentally expose a static function that uses these helpers not realizing they are stateful. ++ to reusing the coreStartServices approach. To help with the props, could you just have one additional prop that is an async |
|
Is this use case basically an internal circular dependency? I wonder if we're finding a clever way to maintain a bad code smell. The code that expresses this problem - is it necessary that it exhibit the problem? -- That aside - Would it make sense to create a new function instead of adding to |
This is what
++ Yes, this is a big consideration. IMO the convenience of being able to import something stateful from a magical
It is not technically a circular dependency in that the use case here is code which is run after kibana/src/core/public/plugins/plugin.ts Lines 121 to 131 in 2d36989 The
So still a core method, but just a separate one? That's certainly an option too; my thinking in updating the existing one is that folks could simply ignore the third item in the array if they don't need it, meaning the current usage still remains unchanged / non-breaking: // most plugins:
const [coreStart, depsStart] = await core.getStartServices();
// plugins who want to receive their own contract:
const [coreStart, depsStart, selfStart] = await core.getStartServices(); |
|
Makes sense to me. |
|
Extending
I was trying to come up with a way to simplify this, but I can't think of anything great. Here are some alternatives in case someone prefers these, but I don't think they significantly change much: Move generic to interface CoreSetup<TPluginsStart extends object = object> {
// ...
getStartServices<
TSelfStart extends object = object
>(): Promise<[CoreStart, TPluginsStart, TSelfStart]>;
}Allow a generic that is an extension of interface CoreSetup<TPluginsStart extends object = object> {
// ...
getStartServices<
TPluginsWithSelfStart extends TPluginsStart = TPluginsStart
>(): Promise<[CoreStart, TPluginsWithSelfStart]>;
}I think I still prefer the original proposal above for clarity and correctness. |
I have no strong feelings either way on each of these approaches. TBH I don't think I even realized |
It does only for |
Both alternative have non-inferable export interface Plugin<
TSetup = void,
TStart = void,
TPluginsSetup extends object = object,
TPluginsStart extends object = object
> {
setup(core: CoreSetup<TPluginsStart, /* added */ TStart>, plugins: TPluginsSetup): TSetup | Promise<TSetup>;
start(core: CoreStart, plugins: TPluginsStart): TStart | Promise<TStart>;
stop?(): void;
} |
|
One other important difference between the That said, now that |
Could you provide an example where this being synchronous is useful? I imagine in most (all?) cases that if this is not populated yet, you can't do whatever it is you need to do with the service, resulting in an error. Is it preferable to throw an exception or return a promise that will resolve once the operation can be completed? If you do still need to have a synchronous API, we could change this to an Observable which has the advantage that subscribers are executed immediately if there is a value (Promises are always on the next tick, so I don't think we can do the same a Promise): let startServices;
core.getStartServices$()
// this callback should be executed immediately if a value is already available
.subscribe(services => (startServices = services));
// emulate previous behavior from createGetterSetter
if (!startServices) {
throw new Error()
} |
Not sure if this is the best example, but one that @alexwizp and I discussed today are the various agg types which get added to a registry during Each registered agg type is provided as a class which extends a base class and has various methods which are exposed on the agg, some of which will need to use start services. Take, for example, the kibana/src/plugins/data/public/search/aggs/buckets/date_histogram.ts Lines 111 to 124 in 5d428b0 This I haven't done a full audit on this particular method -- it may not be that bad to change it around, but hopefully you get the gist.
++ I like this solution much more than |
|
and so on.... |
I'm not a big fan of that kind of approach, as imho this is more of a trick usage by knowing that the consumed observable have a (please feel free to invalidate any or all of that:)
For react components, I don't really see any good arguments TBH I understand that this is just 'easier' to use static module accessors to retrieve these services when needed, however imho this is just an anti-pattern of react philosophy, and as any component rendering is ensured to be done after the whole platform
The usage is not really inside the constructor, as it's used inside the More globally, the |
|
this pretty much means we cannot have any sync method with dependency on start contracts on any items that we add to any registry. its true that the getxxx accessors won't be available any sooner, but we can assume they are available which allows us to leave some code sync and avoid more refactoring. we know that they were set as we only allow registration to our registries in |
|
Initial issue fixed, however I'm reopening to continue the discussion |
|
Cross-posting @streamich's idea for a synchronous way of interacting with |
This "trick" does require a value to have been emitted before being subscribed to, but the behaviour of subscribe handlers executing synchronously is part of the Observables spec. So I think the biggest disadvantage is that the code reads like async code, but is in fact depended upon to be sync which isn't very explicit. In many ways this is like setting a class property in setup and then reading it in start, it's guaranteed to work, but the code doesn't make this explicit. It's also maybe brittle in that a change that seems unrelated can break the behaviour, although throwing as a precaution should quickly surface that. I'll believe #61413 is possible when I see green CI 😛 , the spec definitely says |
|
@lukeelmers I think we can make it easier, without making synchronous the |
|
#61413 Also doesn't take into account that we don't yet have synchronous lifecycles. So a plugin could do Once lifecycles are fully synchronous, we can build all the plugins' start contracts synchronously and therefore make I believe this "start services not ready" exception problem is fundamental. Even though lifecycles are synchronous, Unless we can make all the other core start services sync kibana/src/core/server/server.ts Lines 180 to 207 in 4b012fb it will always be possible to write code that throws this exception with code like setup(core) {
setImmediate(() => {core.getStartServices()});
process.nextTick(() => {core.getStartServices()});
Promise.resolve().then(() => {core.getStartServices()});
}If we want to eliminate these we either have to make Core's start fully synchronous or go back to async lifecycles. Make Core's start fully sync, if it's possible, will require at least moving the registering of saved object migrations into a pre-setup lifecycle or make them static. |
Yea, +1 for that
We discussed at some point to have something like |
@alexwizp I commented on the PR, but I think your proposal there is a step in the right direction in the interim (even if the long term solution ends up being different), as it will consolidate our usage of the getter/setter pattern, making it easier to change down the road. That said, I still don't love the idea of Since we've established that there's still a need for accessing start services synchronously, IMHO the ideal outcome would be for |
|
+1 for core providing getStartServicesOrDie().core
getStartServicesOrDie().pluginsinstead of getStartServicesOrDie()[0]
getStartServicesOrDie()[1]in plugin code we could call it start().plugins.uiActions.getTrigger('RANGE_BRUSH_TRIGGER').exec({})
Alternatively, if Platform team does not want to own the |
|
We discussed about this issue in our team weekly yesterday. The conclusion was that even if none of the listed options seems like good enough for a long term solution, the
The array type would allow the same thing with I think the return type should be an object. My only concern is consistency. |
|
It could returns both ways initially await getStartServices()[0] === await getStartServices().corewith the following work that deprecates await getStartServices()[0] |
Yea, but changing the |
|
as we // before
setup(core, plugins){
plugins.pluginA.register(async () => {
const plugins = await core.getStartServices()[1];
plugins.pluginA.doSomething()
});
}
// after
setup(core, plugins){
plugins.pluginA.register(() => {
const { plugins } = this.startDeps!;
plugins.pluginA.doSomething()
});
}
start(core, plugins) {
this.startDeps = { core, plugins };
}The other alternatives I can think of, requiring changing interface of lifecycles (expose the whole plugin interface at once, for example), removing explicit lifecycles, provide but we lose predictability of the current implementation in this case. |
|
In cases where your The issue is that most of the registries shared between plugins are currently synchronous, and only accept to register resolved objects in a synchronous way, and that during An example of reason to use the sync module-getter trick would be: // before
// some file
import { getModuleStaticService, setModuleStaticService } from './service_getters'
class SomeProvider {
doSomethingSync() {
// will always been executed after `start` anyway
getModuleStaticService().doStuff();
}
}
// plugin file
Plugin {
setup(core, {pluginA}){
pluginA.registerSync(new SomeProvider());
}
start(core) {
setModuleStaticService(core.someService);
}
}The introduction of // after
class SomeProvider {
constructor(private serviceGetter: () => SomeService) {}
doSomethingSync() {
serviceGetter().doStuff();
}
}
setup(core, {pluginA}){
const serviceGetter = () => core.getStartServicesOrDie().someService;
pluginA.register(new SomeProvider(serviceGetter));
}Although, you solution would also work, even if it still forces to use a 'visible' class Plugin {
private startDeps?: CoreAndDeps
setup(core, {pluginA}){
const serviceGetter = () => this.startDeps![0].somePlugin;
pluginA.register(new SomeProvider(serviceGetter));
}
start(core, plugins) {
this.startDeps = { core, plugins };
}
}It's still quite similar technically to what we can provide with a sync core getter. It just feels a little less 'integrated' in core's framework imho. One other option would just be to declare and document that async-registering registries should be the way to go in NP. Something like class SomeRegistry {
private pendingRegistrations: Array<Promise<SomeStuff>> = [];
private entries: SomeStuff[] = [];
setup() {
return {
register: (Promise<SomeStuff>) => pendingRegistrations.push(pendingRegistrations);
}
}
async start() {
const stuffs = await Promise.all(this.pendingRegistrations);
stuffs.forEach(stuff => entries.push(stuff));
return {
getAll: () => entries,
}
}
}Upsides
Downsides
Limitations:
Personally, to have tried to implement that in the SavedObjectsManagement plugin, I did not feel convinced that this was a solid option. |
|
Notes from our meeting: Why this lifecycle & dependency problem exists:
Decision:
|
|
This "manual" approach does not seem useful at all, it only can be used if your code that you are registering is defined inline in the plugin, if the actual registrants are defined in other modules—like it is done in most places in Kibana—this pattern will simply require everyone to implement This will not work: setup(core, plugins){
plugins.pluginA.register(myRegistrant(this.startDeps!));
}
start(core, plugins) {
this.startDeps = { core, plugins };
}Following this "manual" approach, to make it work, the plugin basically needs to implement the getStartServicesOrDie = () => {
if (!this.startDeps) throw 123;
return this.startDeps;
};
setup(core, plugins){
plugins.pluginA.register(myRegistrant(this.getStartServicesOrDie));
}
start(core, plugins) {
this.startDeps = { core, plugins };
}Not sure how useful it is to document this "manual" approach. For us it seems the best approach I can see now is to add |
That's fair & I can get on board with this stance -- However, I do think it's worth pointing out that as long as there are synchronous items being added to registries in Kibana, people will be devising their own workarounds for this whether we like it or not. If we truly want to discourage this usage, we should probably be giving guidance around how folks should design registries to avoid needing sync access to start services in the first place. Otherwise this will be a recurring problem.
If we are taking the stance that we don't want folks accessing start services synchronously, then IMO putting this in At this point my vote would be to pick a pattern for dealing with this and implement it in each plugin individually, as we are only talking about ~3-5 lines of added code. I think @streamich's proposal would work, and is similar to the one @alexwizp had in #61628. To me the biggest benefits are 1) avoiding importing from modules like |
The team is on the same page here. We definitely should be doing that. BTH my async registry example in #61106 (comment) seems to be an option, however this kinda go against the whole sync lifecycle plan, which is why we need to think about that with a wider vision. Also most options include some breaking changes in the core APIs, which is why we don't want to tackle that before 8.0 to avoid introducing factors that will slow down the NP migration for plugins. This is the main reason for the 'lets wait' decision, even if far from perfect.
I agree that this is a risk. The async -> sync bridge is quite straightforward: type StartServicesSyncAccessor<
TPluginsStart extends object = object,
TStart = unknown
> = () => UnwrapPromise<ReturnType<StartServicesAccessor<TPluginsStart, TStart>>>;
// open to naming, `convertToSyncStartServicesAccessor`?
const getStartServicesSyncFactory = <TPluginsStart extends object, TStart>(
accessor: StartServicesAccessor<TPluginsStart, TStart>
): StartServicesSyncAccessor<TPluginsStart, TStart> => {
let contracts: [CoreStart, TPluginsStart, TStart] | undefined;
accessor().then(asyncContracts => {
contracts = asyncContracts;
});
return () => {
if (contracts === undefined) {
throw new Error('trying to access start services before start.'); // or return undefined?
}
return contracts;
};
};Maybe without exposing a startService sync accessor as an 'official' API from @elastic/kibana-platform WDYT? Would exposing the sync converter as a static util be acceptable, or do this go against the |
|
catching up to this discussion and one question comes to mind: @rudolf is above mentioning that at the moment lifecycle methods can still be async, and that once all of them are sync what will actually happen in this case then, if setup can be async ? the setup life cycle will never complete, as we'll be waiting for start lifecycle to complete which should not start until setup is done ? and which plugins actually have async setup methods ? is this something we can refactor soon ? |
That's correct. It's equivalent to the synchronous version in that they both break Kibana. The one will die with an exception, the other hang and never resolve:
We're tracking the work in #53268 but the list of plugins that need refactoring is incomplete. We initially audited all plugins during the Lifecycles RFC and it didn't seem like there were any plugins that required more than a bit of refactoring after we expose a way to read config synchronously. But we haven't done a more recent audit... |
|
FYI to those following along, @streamich has a PR up to add a
@joshdover When we were discussing this as a team, one question that came up was around potential performance repercussions of preferring to access start services asynchronously. As we dove into that topic, we realized that we didn't have a great understanding of why you'd prefer to avoid synchronous usage -- would you be able to expand on this a little so that we have a better grasp of the tradeoffs for future reference? |
The main reason against preferring a sync accessor is that it requires the developer to understand and reason about when that function can be called. Having an API that fails sometimes is not what would I consider a good API design. It also makes accurately testing this difficult. Promises solve this problem by being able to defer execution until the start services are available, which allows the consumer of the API to not need to know underlying implementation details of the API. Though as @ppisljar pointed out above, this isn't quite true since it's possible for a developer to create a situation that would never resolve. In practice, this would trigger the lifecycle timeout that Core implements, resulting in crashing the Kibana server or client application. I could entertain an argument that this failure scenario is actually a worse developer experience than a sync accessor that can fail, since the cause of the timeout is not immediately obvious. If there is consensus that that statement is true, then I'd be comfortable introducing this into Core. I think this entire problem is a code smell that indicates fundamental issues with the general design of the Kibana Platform. We introduced multiple lifecycle methods in order to avoid having to make all of Kibana reactive. However, there are clearly scenarios where this pattern doesn't work well. This is a complicated problem and I think we need to spend dedicated time on solving this issue more holistically. However, I don't think now is the right time do that while so many plugins are still migrating. |
++ Agreed on these points, including that we should revisit this discussion later -- perhaps as the planning around sync lifecycle methods progresses. Thanks for clarifying! In the meantime, I think #63720 should work just fine for us, and is lightweight enough that if a core solution is introduced later it shouldn't be a huge effort to migrate things over to it. On that note, I'll go ahead and close this issue as I think we have achieved the original goal of a better solution to this problem than |
Summary
One recurring challenge we've seen with authoring new platform plugins is finding a way to access a plugin's own
startcontract from within items that are registered in thesetupmethod.This is a particularly common issue for plugins which primarily expose services. One concrete example of this is the
esaggsexpression function in thedataplugin: The function must be registered with theexpressionsplugin in thesetupmethod, but the function itself relies ondata.search,data.indexPatterns, anddata.query.The solution to this problem for accessing
coreor your own plugin's dependencies is to usecore.getStartServices(), but you cannot get your ownstartcontract from there.A Sad Workaround 🙁
In
dataandvisualizationsthis limitation was worked around by usingkibana_utils/create_getter_setterto generate a getter/setter for each service. The setter was then called fromstartso the service could be available via the getter for items registered during setup:This approach is problematic for a number of reasons, including:
datais using setters to manage dependencies internally, however mocked values now need to be added to each of the setters to ensure tests don't break."UiSettings" is not set.ui/new_platformto ensure services get set another time, which is about to get worse as thevisualizationsplugin is migrated.As a result, we are moving away from
createGetterSetterfor dealing withcoreandpluginsdependencies (in favor ofgetStartServices), however the problem still exists for accessing internalstartcontracts.I have also explored a solution which basically re-implements our own version of
getStartServices(@stacey-gammon has done this as well). This is IMO better than the getters/setters, but still added boilerplate for each of our plugins, which could be avoided by...My Proposal 🙂
I'd like to discuss returning a plugin's own
startcontract fromcore.getStartServices():Since
startDependencies$.nextis already called after the plugin'sstartContractis retrieved inPluginWrapper, it seems the value could be easily passed along there:Benefits
coreon the client & server, so folks wouldn't need to pick up any new tools, just retrieve the new value fromgetStartServices.Drawbacks
CoreSetupgeneric. As @pgayvallet pointed out, this would requireCoreSetupto have another parameter as it would need to be aware of a plugin's ownstartContract:Alternatives
Another alternative if we really don't want to touch
getStartServiceswould be providing a recommended pattern for dealing with this use case. Perhaps with a helper which could replace the existingcreateGetterSetterinkibana_utils.Questions
getStartServicesprovided byLegacyPlatformService? I assume this feature would simply not be available in legacy.createCoreSetupMockmay need to be modified so that you could pass mock return values togetStartServices:The text was updated successfully, but these errors were encountered: