Introduce start lifecycle event to client
#35269
Conversation
💔 Build Failed |
💔 Build Failed |
joshdover
changed the title
start lifecycle event to client
💚 Build Succeeded |
joshdover
changed the title
start lifecycle event to clientstart lifecycle event to client
joshdover
added
Team:Core
|
Pinging @elastic/kibana-platform |
joshdover
added
Feature:New Platform
release_note:skip
💔 Build Failed |
💚 Build Succeeded |
|
|
||
| return { fatalErrors }; | ||
| await this.plugins.start(core); |
There was a problem hiding this comment.
Is there any harm in doing these in parallel?
await Promise.all([
this.plugins.start(core),
this.legacyPlatform.start({ core, targetDomElement: legacyPlatformTargetDomElement }),
]);There was a problem hiding this comment.
Yes, though we don't do this yet, I believe we will expose new platform plugin contracts to the legacy platform. We'll need those contracts to be available before we even kick off the legacy code.
src/core/public/injected_metadata/injected_metadata_service.mock.ts
Outdated
Show resolved
Hide resolved
| constructor(private readonly params: LegacyPlatformParams) {} | ||
|
|
||
| public setup(core: CoreSetup) { | ||
| public async setup({ core }: SetupDeps) { |
There was a problem hiding this comment.
I noticed that instead of passing core as an argument to setup it is now passed as a property to this object. Can you explain why this was necessary?
There was a problem hiding this comment.
I did this for consistency, since all the other services accept an option bag. Now that I'm reading this again, I think I'll put it back since CoreSetup is already a bag of services.
| uiSettings, | ||
| chrome, | ||
| } = core; | ||
| // Inject parts of the new platform into parts of the legacy platform | ||
| // so that legacy APIs/modules can mimic their new platform counterparts | ||
| require('ui/new_platform').__newPlatformInit__(core); | ||
| require('ui/new_platform').__newPlatformInitSetup__(core); |
There was a problem hiding this comment.
I feel like Init was the identifier being used to denote faking setup. Thoughts on renaming these identifiers to __newPlatformSetup__ and exposing __newPlatformStart__?
There was a problem hiding this comment.
I guess there are 3 now. Can you explain why we need all of __newPlatformInit__, __newPlatformInitSetup__, and __newPlatformInitStart__ ?
There was a problem hiding this comment.
I think your first suggestion makes sense: __newPlatformSetup__ and __newPlatformStart__. I don't think we currently expose anything to the legacy code when the LegacyService is constructed, so no init should be needed.
| throw new Error(`Plugin "${this.name}" can't be started since it isn't set up.`); | ||
| } | ||
|
|
||
| return await this.instance.start(startContext, plugins); |
There was a problem hiding this comment.
return await is typically redundant outside of try/catch blocks, can you explain the reasoning for awaiting here?
There was a problem hiding this comment.
It's a little redundant, I agree. I think it's a bit matter of opinion here, but I've started to prefer using await on promise return values so that if someone does add try/catch later, they don't forget that what is being called won't throw without an await.
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
Summary
In preparation for the ApplicationService, we need a point in time where we know all applications are registered in order to perform privilege checks and other setup. To support this, I'm re-introducing the
startlifecycle hook to core services and plugins.This comes with a couple caveats when it comes to integrating with legacy browser code:
Some core services are not going to be available at legacy files import time
CoreSetupwill be available when legacy files are imported, butCoreStartwill not be available until just before angular is bootstrapped.For instance,
CapabilitiesServicewill not be able to provide any data until thestartlifecycle event. Any legacy code that needs services that are not available untilstartwill need to execute within an Angular context, at which point these services are guaranteed to be available.Setup contracts can still be used in legacy code after setup is finished
We don't have a mechanism in place that allows us to prevent usage of setup contracts after setup is complete and we are in the "start" part of the lifecycle.
Many of the things in setup may need to moved to start or exposed in start as well as setup. I've done some of that in this PR, but I plan to flesh this out more (specifically, the ChromeService) as the ApplicationService is built out since there are dependencies here.
If the platform team feels comfortable moving forward, I expect this to land around the same time as #35297 which introduces the
startevent on the server as well. At that time, we should also make sure to update the Migration Guide.