-
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
Deprecate kibana_react RedirectAppLinks in favor of shared_ux RedirectAppLinks #128178
Conversation
RedirectAppLinks
in favor of Shared_UX RedirectAppLinks
RedirectAppLinks
in favor of Shared_UX RedirectAppLinks
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.
VisEditors changes LGTM, just adding deprecated markers
@@ -23,6 +23,7 @@ import { createAppNavigationHandler } from '../app_navigation_handler'; | |||
// @ts-expect-error untyped component | |||
import { Synopsis } from '../synopsis'; | |||
import { getServices } from '../../kibana_services'; | |||
/** @deprecated Use `RedirectAppLinks` from `@kbn/shared-ux-components */ |
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 don't think all of these comments are necessary: If you add @deprecated
to the JSDoc comments where RedirectAppLinks
is defined, as well as a @removeBy
version, then the deprecation and all of its usages will automatically be tracked in the deprecated API docs (internal link).
Also, many IDEs will automatically indicate that the API in use is deprecated (vscode example below), so I don't think the comments are adding much.
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.
@rshen91 Just to clarify, I was recommending the following:
- Adding a deprecated (and optionally removeBy) flag where RedirectAppLinks is defined
* @remarks
* It is recommended to use the component at the highest possible level of the component tree that would
* require to handle the links. A good practice is to consider it as a context provider and to use it
* at the root level of an application or of the page that require the feature.
+*
+* @deprecated
+* @removeBy 8.3 // or whatever
*/
- Removing all of these comments (not just from this file, but from the whole PR)
-/** @deprecated Use `RedirectAppLinks` from `@kbn/shared-ux-components */
This way you don't need any CODEOWNERS reviews, and we can use our existing docs tooling to determine where deprecated APIs are in use and track that they are removed on time. (Or, as Julia suggests, it might save y'all a lot of wrangling to just make those changes in this PR directly, since it's only used in a few places).
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.
ah ok I think Im following thank you
Pinging @elastic/fleet (Team:Fleet) |
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.
LGTM, out of curiosity, is there any risk of just updating the imports in this pr? It doesn't look like more effort than adding the deprecation comment.
💚 Build SucceededMetrics [docs]Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: cc @rshen91 |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
2 similar comments
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Summary
This PR is intended to alert teams using the RedirectAppLinks component of the deprecation of the previous implementation. The RedirectAppLinks can now be imported from the shared ux package.
This PR just adds deprecation labels to the current implementations of RedirectAppLinks.
For maintainers