[CM] Client-side content client: setup `update` `delete` `search` methods by Dosant · Pull Request #151041 · elastic/kibana

Dosant · 2023-02-13T16:56:45Z

Summary

Nothing to write home about - just a bit more boilerplate setup code. Method signatures and RPC related interfaces are expected to be changed later with an initial RPC layer implementation.

Follow-up to [CM] Setup client side content client #150171, adding a dummy setup for update delete and search methods. The IN and OUT args will be changes when we glue these to the rpc service and its types
Also clean up unit tests a bit to have setup logic contained in a helper instead of scattered in global scope

Dosant · 2023-02-13T16:58:55Z

src/plugins/content_management/common/rpc.ts

 }

+// -- Update content
+const updateSchemas: ProcedureSchemas = {


Just following the older examples for now. It is clear that these types will change when the real RPC implementation will be implemented.

Dosant · 2023-02-13T16:59:12Z

src/plugins/content_management/public/content_client/content_client.test.ts

-  crudClient = createCrudClientMock();
-  contentClient = new ContentClient(() => crudClient);
-});
+const setup = () => {


Making tests cleaner following @vadimkibana's suggestion from another pr

elasticmachine · 2023-02-14T13:40:49Z

Pinging @elastic/appex-sharedux (Team:SharedUX)

Dosant · 2023-02-15T10:10:40Z

@elasticmachine merge upstream

Dosant · 2023-02-15T11:14:34Z

@elasticmachine merge upstream

sebelga

LGTM! 👍 Left a few comments about naming and exposing hook types with generics. Let me know if that could solve the TS generic problem in your demo app.

sebelga · 2023-02-15T11:45:14Z

src/plugins/content_management/public/content_client/content_client.tsx

  item: (type: string, id: string) => {
    return [...queryKeyBuilder.all(type), id] as const;
  },
+  search: (type: string, params: unknown) => {


Can we call it query instead of params to align with the rpc query naming?

Also, can we now rename type with id as we use id to uniquely identify a ContentType ?

Can we call it query instead of params to align with the rpc query naming?

Sure don't mind, I so you already did that in your pr.

Also, can we now rename type with id as we use id to uniquely identify a ContentType ?

Hm, ContentTypeRegistry id means id of the type. But here the main subject is Content (Content item itself), so id refer to the content. e.g. see item which is a key for get:

item: (type: string, id: string) => {
return [...queryKeyBuilder.all(type), id] as const;
},

maybe it should be?

item: (contentTypeId: string, itemId: string) => {
return [...queryKeyBuilder.all(contentTypeId), itemId] as const;
},

search: (contentTypeId: string, query: unknown) => {

Yes, server side I am using contentTypeId, it seems more consistent to refer to ContentType.id.

sebelga · 2023-02-15T11:46:18Z

src/plugins/content_management/public/content_client/content_client.tsx

    return [...queryKeyBuilder.all(type), id] as const;
  },
+  search: (type: string, params: unknown) => {
+    return [...queryKeyBuilder.all(type), 'search', params] as const;


Shouldn't we JSON.stringify the params? Or is that done under the hood?

Done under the hood

sebelga · 2023-02-15T11:47:03Z

src/plugins/content_management/public/content_client/content_client.tsx

    },
+    search: <I extends SearchIn = SearchIn, O extends SearchOut = SearchOut>(input: I) => {
+      return {
+        queryKey: queryKeyBuilder.search(input.contentType, input.params),


I think this should be input.contentTypeId

In the current types it seems it is input.contentType

export interface SearchIn< T extends string = string, Params extends object = Record<string, unknown>, Options extends object = any > { contentType: T; params: Params; options?: Options; }

I understand that the current type will change so I'd not worry about the current state of them

sebelga · 2023-02-15T12:09:49Z

src/plugins/content_management/public/content_client/content_client_mutation_hooks.tsx

  });
 };
+
+export const useUpdateContentMutation = <I extends UpdateIn = UpdateIn, O = unknown>() => {


I think we want to expose types that unique content can use to declare their mutation (and avoid the problem of declaring the generic everywhere).

Looking at the useCreateContentMutation we would expose

export type CreateContentMutationHook< I extends CreateIn = CreateIn, O = unknown > = UseMutationResult<O, unknown, I, unknown>;

Each content can then expose their mutation types

type CreateFooMutation = CreateContentMutationHook< { contentTypeId: 'foo'; data: { title: string }; // Shape of the input }, { result: 'success' | 'error' } // Shape of the response >;

And when consumed it can be used like this

const createFoo: CreateFooMutation = useCreateContentMutation();

It is properly typed

If that works we can apply the same for queries

Sounds good. I'll try and see if there are any issues

kibana-ci · 2023-02-15T12:16:13Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: c9bdcc8

Failed CI Steps

FTR Configs #53

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`contentManagement`	25	42	+17

Unknown metric groups

API count

id	before	after	diff
`contentManagement`	25	42	+17

History

💔 Build #108141 failed 370b3c0
💚 Build #107843 succeeded 116dd81
💔 Build #107661 failed e8f8396

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…hods (elastic#151041) Nothing to write home about - just a bit more boilerplate setup code. Method signatures and RPC related interfaces are expected to be changed later with an initial RPC layer implementation. - Follow-up to elastic#150171, adding a dummy setup for `update` `delete` and `search` methods. **The `IN` and `OUT` args will be changes when we glue these to the rpc service and its types** - Also clean up unit tests a bit to have setup logic contained in a helper instead of scattered in global scope

setup update delete search methods

e8f8396

Dosant commented Feb 13, 2023

View reviewed changes

fix rpc client

116dd81

Dosant added release_note:skip Skip the PR/issue when compiling release notes Feature:Content Management User generated content (saved objects) management Team:SharedUX Platform AppEx-SharedUX (formerly Global Experience) t// labels Feb 14, 2023

Dosant marked this pull request as ready for review February 14, 2023 13:40

Dosant requested a review from a team as a code owner February 14, 2023 13:40

Dosant requested review from sebelga and vadimkibana February 14, 2023 13:41

Merge branch 'main' into d/2023-02-13-content-client-cache-more-methods

370b3c0

Merge branch 'main' into d/2023-02-13-content-client-cache-more-methods

c9bdcc8

sebelga approved these changes Feb 15, 2023

View reviewed changes

Dosant merged commit 4076ece into elastic:main Feb 15, 2023

kibanamachine added v8.8.0 backport:skip This PR does not require backporting labels Feb 15, 2023

This was referenced Feb 15, 2023

.update() support in cache & browser-side registry #150861

Closed

delete() support in cache & browser-side registry #150863

Closed

Conversation

Dosant commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Dosant Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 14, 2023

Uh oh!

Dosant commented Feb 15, 2023

Uh oh!

Dosant commented Feb 15, 2023

Uh oh!

sebelga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Feb 15, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Public APIs missing comments

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Dosant commented Feb 13, 2023 •

edited

Loading

Dosant Feb 13, 2023 •

edited

Loading

sebelga Feb 15, 2023 •

edited

Loading