-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Remove legacy applications and legacy mode #75987
Remove legacy applications and legacy mode #75987
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
/** @public */ | ||
export interface AppBase { | ||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Diff can be a little hard to read, but I mostly merged AppBase
into App
, as we no longer need this supertype due to the LegacyApp
removal.
// Find the mounter including legacy mounters with subapps: | ||
const [id, mounter] = mounters.has(appId) | ||
? [appId, mounters.get(appId)] | ||
: [...mounters].filter(([key]) => key.split(':')[0] === appId)[0] ?? []; | ||
|
||
const [id, mounter] = mounters.has(appId) ? [appId, mounters.get(appId)] : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially totally removed the id retrieval logic from the catch-all handler, and was just passing an undefined mounter and a 'not-found' id to AppContainer
, however this catch-all is still used to properly dispaly the uptime
app, due to the way they declare their base path:
kibana/x-pack/plugins/uptime/public/apps/plugin.ts
Lines 86 to 89 in 532f2d7
core.application.register({ | |
appRoute: '/app/uptime#/', | |
id: PLUGIN.ID, | |
euiIconType: 'uptimeApp', |
Not sure how or why, but the hashbang in the appRoute make their specific route not matching, the previous routes definition, but the appId is properly identified by the catch-all.
keeping these 3 lines do not have a significant impact anyway, so I did not spend much time trying to fix an issue around hashbang routing, that we officially are not supporting anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we open an issue for the Uptime team to migrate away from hashbang routing and leave a note that we can remove these lines once that is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created #76348 and will add comment.
chrome.getInjected = (name?: string, defaultValue?: any) => | ||
cloneDeep( | ||
name | ||
? newPlatformInjectedVars.getInjectedVar(name, defaultValue) | ||
: newPlatformInjectedVars.getInjectedVars() | ||
); | ||
chrome.getInjected = (name: string, defaultValue?: any) => | ||
cloneDeep(newPlatformInjectedVars.getInjectedVar(name, defaultValue)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted the LegacyCore types, replaced with CoreStart/Setup were needed in the legacy code, so getInjectedVars
is no longer present on the type. Had to adapt that and remove some legacy tests.
This is dead code anyway, and is going to be deleted in #76085
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code owner changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things that still need to be adjusted:
- Remove the
getLegacyMode()
API from InjectedMetadataService (it's also used in a couple places that will need to be updated, eg. chrome_service.tsx). Also remove theLegacyNavLink
type and associated methods. src/core/public/application/types.ts:654
-NavigateToAppOptions.replace
can remove the remark regarding legacy applications- Many of the UI components in
src/core/public/chrome/ui/header
have alegacyMode
prop. Can we remove those? src/core/public/chrome/ui/header/collapsible_nav.test.tsx:43,65
two lines reference legacy flags that were removed
src/core/public/application/types.ts
Outdated
@@ -826,9 +767,3 @@ export interface InternalApplicationStart extends Omit<ApplicationStart, 'regist | |||
*/ | |||
history: History<unknown> | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this still be undefined? Shouldn't it always be present now since we no longer have a legacy mode?
// Find the mounter including legacy mounters with subapps: | ||
const [id, mounter] = mounters.has(appId) | ||
? [appId, mounters.get(appId)] | ||
: [...mounters].filter(([key]) => key.split(':')[0] === appId)[0] ?? []; | ||
|
||
const [id, mounter] = mounters.has(appId) ? [appId, mounters.get(appId)] : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we open an issue for the Uptime team to migrate away from hashbang routing and leave a note that we can remove these lines once that is done?
onAppLeave: expect.any(Function), | ||
history: expect.any(ScopedHistory), | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove the last test too: displays error page if legacy app is inaccessible
?
@@ -54,7 +54,6 @@ describe('AppContainer', () => { | |||
appBasePath: '/base-path', | |||
appRoute: '/some-route', | |||
unmountBeforeMounting: false, | |||
legacy: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's one more below on line 145
* @internal | ||
*/ | ||
legacy?: boolean; | ||
|
||
/** | ||
* Hide the UI chrome when the application is mounted. Defaults to `false`. | ||
* Takes precedence over chrome service visibility settings. | ||
*/ | ||
chromeless?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a new line before the TSDoc below
@@ -71,7 +66,6 @@ export function createEuiListItem({ | |||
if ( | |||
!externalLink && // ignore external links | |||
!legacyMode && // ignore when in legacy mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
coreSystem.stop(); | ||
expect(MockLegacyPlatformService.stop).toHaveBeenCalled(); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Below on line 388 we are still returning a legacyTargetDomElement
unnecessarily
@@ -86,7 +82,6 @@ export interface InternalCoreStart extends Omit<CoreStart, 'application'> { | |||
export class CoreSystem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TSDoc above should remove all references to LegacyPlatform
@@ -142,12 +142,6 @@ export interface ChromeNavLink { | |||
* register an Application if needed. | |||
*/ | |||
readonly hidden?: boolean; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above, on the href
property, can we now make that required?
Can we remove these properties: subUrlBase
, disableSubUrlTracking
, linkToLastSubUrl
?
@joshdover I think I addressed all your comments. Do you see anything else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maps changes lgtm!
- code review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
KibanaApp owned code LGTM, did a checkout in Firefox, no regressions detected. thx for the cleanup!
const chromeUi = chrome.getHeaderComponent(); | ||
const appUi = application.getComponent(); | ||
const bannerUi = overlays.banners.getComponent(); | ||
|
||
const legacyMode = injectedMetadata.getLegacyMode(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you remove legacyMode
leftovers?
legacyMode: false, |
legacyMode: false, |
kibana/src/core/server/rendering/types.ts
Line 48 in 53b1875
legacyMode: boolean; |
There are still a few functional tests referencing the legacy apps
and
kibana/test/plugin_functional/test_suites/core_plugins/applications.ts
Lines 150 to 158 in 384213f
it.skip('can navigate from NP apps to legacy apps', async () => { | |
await appsMenu.clickLink('Stack Management'); | |
await testSubjects.existOrFail('managementNav'); | |
}); | |
it('can navigate from legacy apps to NP apps', async () => { | |
await appsMenu.clickLink('Foo'); | |
await testSubjects.existOrFail('fooAppHome'); | |
}); |
@@ -122,37 +102,11 @@ export interface ChromeNavLink { | |||
* @deprecated | |||
*/ | |||
readonly active?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's deprecated
as well. can be removed now?
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
oss distributable file count
distributable file count
History
To update your PR or re-run it, just comment with: |
* remove legacy mode and legacy service * fix tests * remove txt file * fix xpack code * fix types * remove legacy test * fix integration tests * remove legacy reference * yet another legacy reference * handle uptime app special case * update generated doc * address review comments * remove legacy fields from ChromeNavLink * few fixes * remove legacy API from docTitle service * ChromeNavLink.href is now mandatory * update generated doc * remove legacyMode leftovers * remove ChromeNavLink.active * update generated doc # Conflicts: # src/core/public/chrome/nav_links/to_nav_link.ts # src/core/public/chrome/ui/header/nav_link.tsx # src/core/public/legacy/legacy_service.ts
* remove legacy mode and legacy service * fix tests * remove txt file * fix xpack code * fix types * remove legacy test * fix integration tests * remove legacy reference * yet another legacy reference * handle uptime app special case * update generated doc * address review comments * remove legacy fields from ChromeNavLink * few fixes * remove legacy API from docTitle service * ChromeNavLink.href is now mandatory * update generated doc * remove legacyMode leftovers * remove ChromeNavLink.active * update generated doc # Conflicts: # src/core/public/chrome/nav_links/to_nav_link.ts # src/core/public/chrome/ui/header/nav_link.tsx # src/core/public/legacy/legacy_service.ts
…nes/processors-forms-k-s * 'master' of github.com:elastic/kibana: (216 commits) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) Replace FetchOptions with ISearchOptions (elastic#76538) Move streams utils to the core (elastic#76397) [Ingest Manager] Wording & linking improvements (elastic#76284) remove legacy kibana plugin (elastic#76064) [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379) Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482) [APM] Service maps layout enhancements (elastic#76481) [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586) [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544) Index Pattern class - remove toJSON and toString (elastic#76246) skip failing suite (elastic#76581) Split up and clarify Enterprise Search codeowners (elastic#76580) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processor_settings_fields.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/foreach.tsx
…oleysens/kibana into ingest-pipelines/processors-forms-k-s * 'ingest-pipelines/processors-forms-k-s' of github.com:jloleysens/kibana: (216 commits) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) Replace FetchOptions with ISearchOptions (elastic#76538) Move streams utils to the core (elastic#76397) [Ingest Manager] Wording & linking improvements (elastic#76284) remove legacy kibana plugin (elastic#76064) [Form lib] Fix regression on field not being validated after reset to its default value. (elastic#76379) Legacy SO import: Fix bug causing multiple overrides to only show the last confirm modal (elastic#76482) [APM] Service maps layout enhancements (elastic#76481) [Security Solution][Detection Engine] Remove RuleTypeSchema in favor of Type for TypeScript (elastic#76586) [Security Solution][Exceptions] - Updates enum schema and tests (elastic#76544) Index Pattern class - remove toJSON and toString (elastic#76246) skip failing suite (elastic#76581) Split up and clarify Enterprise Search codeowners (elastic#76580) ...
* master: (340 commits) [data.search.SearchSource] Remove legacy ES client APIs. (elastic#75943) [release notes] automatically retry on Github API 5xx errors (elastic#76447) [es_ui_shared] Fix eslint exhaustive deps rule (elastic#76392) [i18n] Integrate 7.9.1 Translations (elastic#76391) [APM] Update aggregations to support script sources (elastic#76429) [Security Solution] Refactor Network Top Countries to use Search Strategy (elastic#76244) Document security settings available on ESS (elastic#76513) [Ingest Manager] Add input revision to the config send to the agent (elastic#76327) [DOCS] Identifies cloud settings for Monitoring (elastic#76579) [DOCS] Identifies Cloud settings in Dev Tools (elastic#76583) [Ingest Manager] Better default value for fleet long polling timeout (elastic#76393) [data.indexPatterns] Fix broken rollup index pattern creation (elastic#76593) [Ingest Manager] Split Registry errors into Connection & Response (elastic#76558) [Security Solution] add an excess validation instead of the exact match (elastic#76472) Introduce TS incremental builds & move src/test_utils to TS project (elastic#76082) fix bad merge (elastic#76629) [Newsfeed] Ensure the version format when calling the API (elastic#76381) remove server_extensions mixin (elastic#76606) Remove legacy applications and legacy mode (elastic#75987) [Discover] Fix sidebar element focus behavior when adding / removing columns (elastic#75749) ...
Summary
Fix #74911
Remove the legacy mode from the application service, delete the (client-side) legacy service, and adapt surrounding code.
Checklist