Create common and api types packages for the savedObjects domain #136722
Conversation
pgayvallet
added
Team:Core
pgayvallet
changed the title
|
@elasticmachine merge upstream |
| @@ -50,7 +50,7 @@ interface EncryptedSavedObjectsClientOptions { | |||
| export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientContract { | |||
| constructor( | |||
| private readonly options: EncryptedSavedObjectsClientOptions, | |||
| public readonly errors = options.baseClient.errors | |||
| public readonly errors = SavedObjectsErrorHelpers | |||
There was a problem hiding this comment.
But just to avoid potentially breaking stuff, I kept it (as the original SO client also still have the property even if not defined on the interface)
👍 We'll probably get rid of this in the scope of #134395 anyway
| @@ -8,7 +8,7 @@ | |||
| import { EuiHorizontalRule, EuiText } from '@elastic/eui'; | |||
| import React, { useMemo } from 'react'; | |||
|
|
|||
| import type { SavedObjectReferenceWithContext } from '@kbn/core/public'; | |||
| import type { SavedObjectReferenceWithContext } from '@kbn/core-saved-objects-api-server'; | |||
There was a problem hiding this comment.
question: I see the issue description mentions that it's expected that @kbn/core-saved-objects-api-browser depends on @kbn/core-saved-objects-api-server, does it also apply to the type dependencies from the browser parts of other plugins to @kbn/core-saved-objects-api-server or is it just intermediate step and we'll eventually have SavedObjectReferenceWithContext and SavedObjectsCollectMultiNamespaceReferencesResponse exposed from either @kbn/core-saved-objects-api-browser or @kbn/core-saved-objects-common?
There was a problem hiding this comment.
Good question, to which we don't have a definitive answer atm. For now, the idea is that 'server' types (understand types coming from the server-side client/repository signatures) that are not directly used by publicly exposed Core public-side SO APIs are considered as 'server-side only' from Core standpoint, and therefor will not be exposed from public/common core packages.
Note that, as explained in the issue's description, it's still fine to import such types coming from -server packages from the client side from browser-side plugins that uses it.
Mid/long term, we'll probably want to introduce an additional layer of types for the param/response types of the SO HTTP APIs (which we currently don't have atm). Those types will be exposed from a common package, given they handle the client/server contract via these HTTP APIs.
There was a problem hiding this comment.
Mid/long term, we'll probably want to introduce an additional layer of types for the param/response types of the SO HTTP APIs (which we currently don't have atm). Those types will be exposed from a common package, given they handle the client/server contract via these HTTP APIs.
Got it, it makes sense. Thank you for clarifying.
…n-types-to-package
pgayvallet
force-pushed
the
kbn-135820-so-common-types-to-package
branch
from
745b220 to
8451d9b
Compare
…n-types-to-package
…n-types-to-package
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Any counts in public APIs
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
Unreferenced deprecated APIs
History
To update your PR or re-run it, just comment with: |
Summary
Part 1 of #135820
Required for #134122
Extract all the types associated to the browser and server-side
SavedObjectClient/SavedObjectClientContractandSavedObjectRepositoryand move them to packages.Create the following packages:
@kbn/core-saved-objects-common@kbn/core-saved-objects-api-server@kbn/core-saved-objects-api-browserNote: this PR does not move all the SO types to packages, only the subset associated with the browser/server-side clients. The other types will be done in a follow-up (part 2), then the implementation will (part 3).
Remarks:
-
@kbn/core-saved-objects-api-browserdepends on@kbn/core-saved-objects-api-serverA lot of client-side types were inferred from their server-side counterpart, and move importantly, some were inferred from the server-side client type itself, which would have meant that if we wanted to move these 'common' types to a common package, the whole server-side types would have needed to be in it.
After a lot of thinking and discussions with various people, the conclusion was that the best approach was probably to KISS and just have the browser-side type api types package depends on the server-side one, 'as it's currently done'.
Note that this doesn't go against the package architecture rules in any way, as the Operations teams is not planning on banning cross environment type only imports.
- There will be more than one type package for the SO types
@kbn/core-saved-objects-api-serverand@kbn/core-saved-objects-api-browseronly contain the types associated with the SO clients/repository. The other public types will be extracted to distinct@kbn/core-saved-objects-browserand@kbn/core-saved-objects-api-serverpackages during the next stage of the issue.This separation felt just logical for multiple reasons:
Notable changes:
1. (browser+server) cleanup of the client/repository API types.
The
SavedObjectClientContractandISavedObjectRepository(we're so consistent in naming our interfaces...) were implicitly inferred from their respective concrete implementations (which is something that is not possibleto have for the package arch). This is no longer the case, as explicit interfaces were introduced in this PR.The API types were also properly reorganised (it was so a mess previously), with one file per API, in the
@kbn/core-saved-objects-api-serverand@kbn/core-saved-objects-api-browserpackages.2. (browser) expose
SimpleSavedObjectas an interfaceThe
SimpleSavedObjectconcrete implementation was used in some of the client-side SO client signatures, and exported from our client-side entrypoint (as a concrete class, not as a type). It's now properly exposed as an interface.The implementation is still exported (as
SimpleSavedObjectImpl) atm, as a few test files outside of Core were using the implementation for testing purposes. We could eventually introduce a mock later on to address this point.3. (server) removal of
SavedObjectClientContract.errorsThe
SavedObjectErrorHelperutility has been historically exposed viaSavedObjectClientContract.errors. Cleaning up the types was a good opportunity to remove it from the server-side client type. Adapted the code of the few actors that were accessing it to import and useSavedObjectErrorHelperdirectly instead (this class will be moved to a utility package at some point)4. (browser) removal of some 'server-side only' export from
src/core/publicSome server-side SO API types were exported from Core's client-side entrypoint just because some external consumers were importing it to shape some responses from server APIs that did not belong to Core (so these types were technically not used on the client-side by core public APIs).
These exports were removed, and the imports external to Core were adapted to import the client-side type counterparts, or to import from the server-side api package instead (which is and will be technically allowed by the package system).