Skip to content
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.

Sign up for GitHub

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

[deprecations] Remove/update core-api deprecations targeted for removal by 8.8 #147723

Merged
merged 17 commits into from
Jan 22, 2023
Merged

[deprecations] Remove/update core-api deprecations targeted for removal by 8.8 #147723

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
a5cf52c
replace onAppLeave deprecation with remarks
TinaHeiligers Dec 16, 2022
6c29d7b
removes AppLeaveHandler deprecation label
TinaHeiligers Dec 16, 2022
c6b6d90
Remove OpsMetrics.process deprecation
TinaHeiligers Dec 16, 2022
4149cf3
Removes deprecated SavedObjectsMigrationLogger.warning
TinaHeiligers Dec 16, 2022
902d37d
Adds removeBy 9 to SavedObjectsAttributes as a way to encourage type-…
TinaHeiligers Dec 16, 2022
3754f1e
Handle removing logger.warning
TinaHeiligers Dec 16, 2022
878d650
Update logger.warning to logger.warn
TinaHeiligers Dec 17, 2022
936d7db
Merge branch 'main' into kbn-142915-handle-core-deprecations-for-8
kibanamachine Dec 17, 2022
7188c50
Merge branch 'main' into kbn-142915-handle-core-deprecations-for-8
kibanamachine Dec 19, 2022
7496776
Address some PR comments
TinaHeiligers Dec 19, 2022
a90db2a
Added prefered use of ScopedHistory.block over onAppLeave
TinaHeiligers Dec 19, 2022
dc1a829
remove comment from ts-expect-error
TinaHeiligers Dec 19, 2022
70f6251
removes ts error
TinaHeiligers Dec 19, 2022
1ea8938
removes deprecated param from migration_logger
TinaHeiligers Dec 20, 2022
befb85c
updates migration mocks
TinaHeiligers Dec 20, 2022
b00ee81
Merge branch 'main' into kbn-142915-handle-core-deprecations-for-8
kibanamachine Dec 20, 2022
aba179b
Merge branch 'main' into kbn-142915-handle-core-deprecations-for-8
TinaHeiligers Jan 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/core/application/core-application-browser/src/app_leave.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,6 @@ import type { EuiButtonColor } from '@elastic/eui';
*
* See {@link AppMountParameters} for detailed usage examples.
*
* @public
* @deprecated {@link AppMountParameters.onAppLeave} has been deprecated in favor of {@link ScopedHistory.block}
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
* @removeBy 8.8.0
*/
export type AppLeaveHandler = (
factory: AppLeaveActionFactory,
Expand Down
8 changes: 3 additions & 5 deletions packages/core/application/core-application-browser/src/app_mount.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,6 @@ export interface AppMountParameters<HistoryLocationState = unknown> {
* The route path for configuring navigation to the application.
* This string should not include the base path from HTTP.
*
* @deprecated Use {@link AppMountParameters.history} instead.
* @removeBy 8.8.0
*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this misplaced? Can't we remove appBasePath? Is it not really deprecated?

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Dec 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Um, at first I thought some teams were blocked on using AppMountParams.history over AppMountParams.appBasePath because they couldn't use ScopedHistory.bock over onAppLeave for back button navigation in the browser.

Now though, I'm not so sure.

We have 2 options:

  1. Leave the deprecation in place until teams still using appBasePath migrate away from it
    e.g src/plugins/kibana_overview/public/application.tsx: or
  2. Carry on supporting appBasePath since we're not deprecating onAppLeave or AppLeaveHandler.

@pgayvallet could you recommend the best path forward please?

@afharo For this PR, I'll add the deprecation warning back in unless the recommendation is to continue supporting it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC appBasePath exposition isn't directly related to the ScopedHistory.block thingy. It's exposed because applications based on the legacy (Hash) router needs to know about it to instantiate their own router. I think nothing would block them from migrating to the new (HTML5) router.

Q/A:
should it be deprecated? ihmo yes.
would it make sense to delete it? ihmo yes.
can we delete it? no, as other teams are still using it...
can we easily adapt the code ourselves to delete it? no, migrating from one router to the other is all but trivial, and multiple apps are still using it IIRC
so, what should we do? not sure. Removing the deprecation may be fine, idk.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bit of time to see if teams have the capacity to migrate their routers and don't need to make a decision right now.
If there's no actual harm in keeping appBasePath around then we can treat it in the same way as onAppLeave: remove the deprecation warning even though it's not ideal.

* @example
*
* How to configure react-router with a base path:
Expand Down Expand Up @@ -165,8 +162,9 @@ export interface AppMountParameters<HistoryLocationState = unknown> {
* }
* ```
*
* @deprecated {@link ScopedHistory.block} should be used instead.
* @removeBy 8.8.0
* @remarks Resources with names containing percent sign with other special characters or
* containing `%25` sequence can experience navigation issues. Refs https://github.com/elastic/kibana/issues/82440 and https://github.com/elastic/kibana/issues/132600
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved

*/
onAppLeave: (handler: AppLeaveHandler) => void;

Expand Down
3 changes: 1 addition & 2 deletions packages/core/metrics/core-metrics-server/src/metrics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,7 @@ export interface OpsMetrics {
elasticsearch_client: ElasticsearchClientsMetrics;
/**
* Process related metrics.
* @deprecated use the processes field instead.
* @removeBy 8.8.0
* @remarks processes field preferred
*/
process: OpsProcessMetrics;
/** Process related metrics. Reports an array of objects for each kibana pid.*/
Expand Down
1 change: 1 addition & 0 deletions packages/core/saved-objects/core-saved-objects-common/src/saved_objects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export type SavedObjectAttribute = SavedObjectAttributeSingle | SavedObjectAttri
*
* @public
* @deprecated This type reduces the type safety of your code. Create an interface for your specific saved object type or use `unknown` instead.
* @removeBy 9.0.0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a removeBy version to encourage type-owners to implement their interfaces. If there is a 9.0.0 version, we'll probably have a bunch more deprecations to worry about and shouldn't be carrying existing ones over to a new major.

*/
export interface SavedObjectAttributes {
[key: string]: SavedObjectAttribute;
Expand Down
5 changes: 0 additions & 5 deletions packages/core/saved-objects/core-saved-objects-server/src/migration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,6 @@ export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes
export interface SavedObjectsMigrationLogger {
debug: (msg: string) => void;
info: (msg: string) => void;
/**
* @deprecated Use `warn` instead.
* @removeBy 8.8.0
*/
warning: (msg: string) => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also adapt the implementation accordingly (MigrationLogger in

kibana/packages/core/saved-objects/core-saved-objects-migration-server-internal/src/core/migration_logger.ts

Line 19 in 34c228b

export class MigrationLogger implements SavedObjectsMigrationLogger {
)

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Dec 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thanks!

For the record, the discrepancy in log level type between 'SavedObjectsMigrationLogger' and 'MigrationLogLevel':

export interface SavedObjectsMigrationLogger {
  debug: (msg: string) => void;
  info: (msg: string) => void;
  warn: (msg: string) => void;
  error: <Meta extends LogMeta = LogMeta>(msg: string, meta: Meta) => void;
}

vs

/** @internal */
export type MigrationLogLevel = 'error' | 'info' | 'warning';

Is intentional. The log level gets cast from warning to warn in logStateTransition:

        case 'warning':
          return logger.warn(logMessagePrefix + message);

warn: (msg: string) => void;
error: <Meta extends LogMeta = LogMeta>(msg: string, meta: Meta) => void;
}
Expand Down
4 changes: 2 additions & 2 deletions ...ugins/dashboard/server/dashboard_saved_object/migrations/migrate_to_730/migrations_730.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export const migrations730 = (doc: DashboardDoc700To720, { log }: SavedObjectMig
moveFiltersToQuery(searchSource)
);
} catch (e) {
log.warning(
log.warn(
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
`Exception @ migrations730 while trying to migrate dashboard query filters!\n` +
`${e.stack}\n` +
`dashboard: ${inspect(doc, false, null)}`
Expand All @@ -80,7 +80,7 @@ export const migrations730 = (doc: DashboardDoc700To720, { log }: SavedObjectMig

delete doc.attributes.uiStateJSON;
} catch (e) {
log.warning(
log.warn(
`Exception @ migrations730 while trying to migrate dashboard panels!\n` +
`Error: ${e.stack}\n` +
`dashboard: ${inspect(doc, false, null)}`
Expand Down
1 change: 1 addition & 0 deletions test/analytics/tests/analytics_from_the_browser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {

it('should call flush when using the window-exposed flush method', async () => {
await browser.execute(() => window.__kbnAnalytics.flush());
// @ts-ignore-next-line Property 'getFlushAction' does not exist on type '{ getActionsUntilReportTestPluginLifecycleEvent:
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
const action = await browser.execute(() => window.__analyticsPluginA__.getFlushAction());
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
expect(action).to.eql({ action: 'flush', meta: {} });
});
Expand Down