-
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
[Saved Objects] Browser-side client deprecation notice #148979
[Saved Objects] Browser-side client deprecation notice #148979
Conversation
…om common with deprecation notice for now
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.
🥳
Pinging @elastic/kibana-core (Team:Core) |
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.
Some nits about exports and naming, otherwise nice work!
packages/core/saved-objects/core-saved-objects-api-browser/src/apis/bulk_create.ts
Outdated
Show resolved
Hide resolved
type: string; | ||
id: string; | ||
} | ||
import * as serverTypes from './will_move_to_server'; |
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.
Prefer named imports/exports, especially in packages.
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 think there are a few reasons why it is OK to break convention in this case:
- We are not
*
exporting: we are still selectively exposing a set of types for public use - A namespace serves the function of
as
ing, but in a more efficient way in this instance (IMO) - I expect this code will change/be removed soon anyway (once we remove the SO client from browsers)
Happy to revisit if you feel strongly!
* @public | ||
* @deprecated See https://github.com/elastic/dev/issues/2194 | ||
*/ | ||
export type SavedObjectAttributeSingle = serverTypes.SavedObjectAttributeSingle; |
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.
Honestly, I'm not a fan of this syntax. Using named imports:
import {
SavedObjectAttributeSingle as LegacySavedObjectAttributeSingle,
SavedObjectAttribute as LegacySavedObjectAttribute,
SavedObjectAttributes as LegacySavedObjectAttributes,
SavedObjectReference as LegacySavedObjectReference,
SavedObject as LegacySavedObject,
} from './will_move_to_server';
allows us to use export type SavedObjectAttributeSingle = LegacySavedObjectAttributeSingle;
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.
Same response as #148979 (comment) :)
packages/core/saved-objects/core-saved-objects-common/src/will_move_to_server.ts
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
References to deprecated APIs
Unreferenced deprecated APIs
History
To update your PR or re-run it, just comment with: |
## Summary In the near future we will remove Saved Object (SO) HTTP APIs. This PR deprecates all browser-side SO types and interfaces. General comments on the approach taken here: * Add a deprecation notice that links to a GitHub issue that can be referenced by all teams * Do not break existing imports/exports * Mocks are also an indication of SO browser-side use, added deprecation notices * Some common types must also be deprecated, some may remain. For those to be removed they are moved to a separate file, with a deprecated type-alias re-exported ## Notes to reviewers * Easiest way to get an overview of the changes is to have the file-tree open in the "Files changed" view * Are there any other ways browser-side code can get knowledge of Saved Objects? * Please go through some client code and test that the DX is working as expected (the GitHub issue is discoverable) ## Related * elastic#147150 * elastic/dev#2194 Co-authored-by: kibanamachine <[email protected]>
Ref #149288 |
… avoid deprecation (#149289) ## Summary After merging #148979 there are a number of imports that can be fixed immediately to address our new deprecation notice. ## To Core reviewers The package `core-saved-objects-server` is using types from `core-saved-objects-api-server` which creates a circular dependency when using `SavedObject` type from it's new home in `core-saved-object-server`: `core-saved-objects-server` -> `core-saved-objects-api-server` -> `core-saved-objects-server` One solution is that we can create a new package `packages/core/saved-objects/core-saved-objects-server-shared` that will only hold the `SavedObject` type and a select few others. I'm not sure what the best approach here is. I have left `core-saved-objects-api-server` unchanged for now (i.e., it is still importing `SavedObject` from `common` which is deprecated). Any input would be greatly appreciated! Co-authored-by: kibanamachine <[email protected]>
@jloleysens We forgot to update the docs when deprecating the APIs: https://www.elastic.co/guide/en/kibana/8.6/saved-objects-service.html#_client_side_usage_2 |
) ## Summary After merging #148979 there are a few interfaces in the server-side code that export a reference to a deprecated SO type. In this PR we fix them by updating a few imports to use the non-deprecated SO type. Additionally, we resolve a potential circular reference issue between `core-saved-objects-server` and `core-saved-objects-api-server` by "moving" saved objects and related types exports to `core-saved-objects-api-server`
Summary
In the near future we will remove Saved Object (SO) HTTP APIs. This PR deprecates all browser-side SO types and interfaces.
General notes on the approach taken:
Notes to reviewers
Related