Move application.applications$ to public contract #67463
Conversation
pgayvallet
added
v7.9.0
v8.0.0
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
pgayvallet
added
release_note:skip
src/core/public/application/types.ts
Outdated
| export type AppInfo = Omit<App, 'mount' | 'updater$'> & { | ||
| legacy: false; | ||
| }; |
There was a problem hiding this comment.
Was not very inspired when choosing a name for these types. Any better suggestion is welcome
There was a problem hiding this comment.
PublicAppInfo to indicate that's publicly available to other plugins?
src/core/public/application/utils.ts
Outdated
| export function getAppInfo(app: App<unknown> | LegacyApp): AppInfo | LegacyAppInfo { | ||
| if (isLegacyApp(app)) { | ||
| const { updater$, ...infos } = app; | ||
| return { | ||
| ...infos, |
There was a problem hiding this comment.
I tried a generic approach to have proper return type depending on the input
function getAppInfo<T extends App<unknown> | LegacyApp>(
app: T
): T extends App ? AppInfo : LegacyAppInfo {
if (isLegacyApp(app)) {
const { updater$, ...infos } = app;
return {
...infos,
legacy: true,
};
}
const { updater$, mount, ...infos } = app;
return {
...infos,
legacy: false,
};
}However the isLegacyApp typeguard was not enough for TS, and the return type was not correctly inferred, resulting in type errors. As this is only used to populate a Map<string, AppInfo | LegacyAppInfo anyway, I think current signature is still fine.
There was a problem hiding this comment.
I think it's fine too. Also there are only two remaining legacy apps right now, so we're close to gutting this anyways 😄
|
retest |
src/core/public/public.api.md
Outdated
| subUrlBase?: string; | ||
| } | ||
|
|
||
| // Warning: (ae-incompatible-release-tags) The symbol "LegacyAppInfo" is marked as @public, but its signature references "LegacyApp" which is marked as @internal |
There was a problem hiding this comment.
might want to fix this? not sure if very important though
There was a problem hiding this comment.
LegacyApp was flagged as internal, however LegacyAppInfo is based on it, and is public. What should I do, change LegacyApp visibility to public?
There was a problem hiding this comment.
I guess so. It doesn't matter a whole lot since this is going away soon.
|
retest |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* expose applications$ on public contract * review comments
Summary
Fix #67302
applications$observable on the service's public start contractAppandLegacyAppto avoid leaking 'private' properties such as themountfunctionChecklist