Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 10 additions & 1 deletion src/plugins/content_management/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,14 @@
*/

export { PLUGIN_ID, API_ENDPOINT } from './constants';
export type { ProcedureSchemas, ProcedureName, GetIn, CreateIn } from './rpc';
export type {
ProcedureSchemas,
ProcedureName,
GetIn,
CreateIn,
SearchIn,
SearchOut,
DeleteIn,
UpdateIn,
} from './rpc';
export { procedureNames, schemas as rpcSchemas } from './rpc';
78 changes: 77 additions & 1 deletion src/plugins/content_management/common/rpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export interface ProcedureSchemas {
out?: Type<any> | false;
}

export const procedureNames = ['get', 'create'] as const;
export const procedureNames = ['get', 'create', 'update', 'delete', 'search'] as const;

export type ProcedureName = typeof procedureNames[number];

Expand Down Expand Up @@ -64,9 +64,85 @@ export interface CreateIn<
options?: Options;
}

// -- Update content
const updateSchemas: ProcedureSchemas = {
Copy link
Copy Markdown
Contributor Author

@Dosant Dosant Feb 13, 2023

Choose a reason for hiding this comment

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

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

in: schema.object(
{
contentType: schema.string(),
data: schema.object({}, { unknowns: 'allow' }),
options: schema.maybe(schema.object({}, { unknowns: 'allow' })),
},
{ unknowns: 'forbid' }
),
out: schema.maybe(schema.object({}, { unknowns: 'allow' })),
};

export interface UpdateIn<
T extends string = string,
Data extends object = Record<string, unknown>,
Options extends object = any
> {
contentType: T;
data: Data;
options?: Options;
}

// -- Delete content
const deleteSchemas: ProcedureSchemas = {
in: schema.object(
{
contentType: schema.string(),
data: schema.object({}, { unknowns: 'allow' }),
options: schema.maybe(schema.object({}, { unknowns: 'allow' })),
},
{ unknowns: 'forbid' }
),
out: schema.maybe(schema.object({}, { unknowns: 'allow' })),
};

export interface DeleteIn<
T extends string = string,
Data extends object = Record<string, unknown>,
Options extends object = any
> {
contentType: T;
data: Data;
options?: Options;
}

// -- Search content
const searchSchemas: ProcedureSchemas = {
in: schema.object(
{
contentType: schema.string(),
data: schema.object({}, { unknowns: 'allow' }),
options: schema.maybe(schema.object({}, { unknowns: 'allow' })),
},
{ unknowns: 'forbid' }
),
out: schema.object({ hits: schema.arrayOf(schema.object({}, { unknowns: 'allow' })) }),
};

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

export interface SearchOut<Data extends object = Record<string, unknown>> {
hits: Data[];
}

export const schemas: {
[key in ProcedureName]: ProcedureSchemas;
} = {
get: getSchemas,
create: createSchemas,
update: updateSchemas,
delete: deleteSchemas,
search: searchSchemas,
};
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,19 @@

import { lastValueFrom } from 'rxjs';
import { takeWhile, toArray } from 'rxjs/operators';
import type { CrudClient } from '../crud_client';
import { createCrudClientMock } from '../crud_client/crud_client.mock';
import { ContentClient } from './content_client';
import type { GetIn, CreateIn } from '../../common';
import type { GetIn, CreateIn, UpdateIn, DeleteIn, SearchIn, SearchOut } from '../../common';

let contentClient: ContentClient;
let crudClient: jest.Mocked<CrudClient>;
beforeEach(() => {
crudClient = createCrudClientMock();
contentClient = new ContentClient(() => crudClient);
});
const setup = () => {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

const crudClient = createCrudClientMock();
const contentClient = new ContentClient(() => crudClient);
return { crudClient, contentClient };
};

describe('#get', () => {
it('calls rpcClient.get with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: GetIn = { id: 'test', contentType: 'testType' };
const output = { test: 'test' };
crudClient.get.mockResolvedValueOnce(output);
Expand All @@ -30,6 +29,7 @@ describe('#get', () => {
});

it('calls rpcClient.get$ with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: GetIn = { id: 'test', contentType: 'testType' };
const output = { test: 'test' };
crudClient.get.mockResolvedValueOnce(output);
Expand All @@ -52,6 +52,7 @@ describe('#get', () => {

describe('#create', () => {
it('calls rpcClient.create with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: CreateIn = { contentType: 'testType', data: { foo: 'bar' } };
const output = { test: 'test' };
crudClient.create.mockResolvedValueOnce(output);
Expand All @@ -60,3 +61,59 @@ describe('#create', () => {
expect(crudClient.create).toBeCalledWith(input);
});
});

describe('#update', () => {
it('calls rpcClient.update with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: UpdateIn = { contentType: 'testType', data: { id: 'test', foo: 'bar' } };
const output = { test: 'test' };
crudClient.update.mockResolvedValueOnce(output);

expect(await contentClient.update(input)).toEqual(output);
expect(crudClient.update).toBeCalledWith(input);
});
});

describe('#delete', () => {
it('calls rpcClient.delete with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: DeleteIn = { contentType: 'testType', data: { id: 'test' } };
const output = { test: 'test' };
crudClient.delete.mockResolvedValueOnce(output);

expect(await contentClient.delete(input)).toEqual(output);
expect(crudClient.delete).toBeCalledWith(input);
});
});

describe('#search', () => {
it('calls rpcClient.search with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: SearchIn = { contentType: 'testType', params: {} };
const output: SearchOut = { hits: [{ test: 'test' }] };
crudClient.search.mockResolvedValueOnce(output);
expect(await contentClient.search(input)).toEqual(output);
expect(crudClient.search).toBeCalledWith(input);
});

it('calls rpcClient.search$ with input and returns output', async () => {
const { crudClient, contentClient } = setup();
const input: SearchIn = { contentType: 'testType', params: {} };
const output: SearchOut = { hits: [{ test: 'test' }] };
crudClient.search.mockResolvedValueOnce(output);
const search$ = contentClient.search$(input).pipe(
takeWhile((result) => {
return result.data == null;
}, true),
toArray()
);

const [loadingState, loadedState] = await lastValueFrom(search$);

expect(loadingState.isLoading).toBe(true);
expect(loadingState.data).toBeUndefined();

expect(loadedState.isLoading).toBe(false);
expect(loadedState.data).toEqual(output);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@
import { QueryClient } from '@tanstack/react-query';
import { createQueryObservable } from './query_observable';
import type { CrudClient } from '../crud_client';
import type { CreateIn, GetIn } from '../../common';
import type { CreateIn, GetIn, UpdateIn, DeleteIn, SearchIn, SearchOut } from '../../common';

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

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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) => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

return [...queryKeyBuilder.all(type), 'search', params] as const;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done under the hood

},
};

const createQueryOptionBuilder = ({
Expand All @@ -30,6 +33,12 @@ const createQueryOptionBuilder = ({
queryFn: () => crudClientProvider(input.contentType).get<I, O>(input),
};
},
search: <I extends SearchIn = SearchIn, O extends SearchOut = SearchOut>(input: I) => {
return {
queryKey: queryKeyBuilder.search(input.contentType, input.params),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be input.contentTypeId

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

queryFn: () => crudClientProvider(input.contentType).search<I, O>(input),
};
},
};
};

Expand All @@ -55,4 +64,20 @@ export class ContentClient {
create<I extends CreateIn, O = unknown>(input: I): Promise<O> {
return this.crudClientProvider(input.contentType).create(input);
}

update<I extends UpdateIn, O = unknown>(input: I): Promise<O> {
return this.crudClientProvider(input.contentType).update(input);
}

delete<I extends DeleteIn, O = unknown>(input: I): Promise<O> {
return this.crudClientProvider(input.contentType).delete(input);
}

search<I extends SearchIn, O extends SearchOut = SearchOut>(input: I): Promise<O> {
return this.crudClientProvider(input.contentType).search(input);
}

search$<I extends SearchIn, O extends SearchOut = SearchOut>(input: I) {
return createQueryObservable(this.queryClient, this.queryOptionBuilder.search<I, O>(input));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,32 @@ import React from 'react';
import { renderHook } from '@testing-library/react-hooks';
import { ContentClientProvider } from './content_client_context';
import { ContentClient } from './content_client';
import { CrudClient } from '../crud_client';
import { createCrudClientMock } from '../crud_client/crud_client.mock';
import { useCreateContentMutation } from './content_client_mutation_hooks';
import type { CreateIn } from '../../common';

let contentClient: ContentClient;
let crudClient: jest.Mocked<CrudClient>;
beforeEach(() => {
crudClient = createCrudClientMock();
contentClient = new ContentClient(() => crudClient);
});
import {
useCreateContentMutation,
useUpdateContentMutation,
useDeleteContentMutation,
} from './content_client_mutation_hooks';
import type { CreateIn, UpdateIn, DeleteIn } from '../../common';

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

const Wrapper: React.FC = ({ children }) => (
<ContentClientProvider contentClient={contentClient}>{children}</ContentClientProvider>
);

const Wrapper: React.FC = ({ children }) => (
<ContentClientProvider contentClient={contentClient}>{children}</ContentClientProvider>
);
return {
Wrapper,
contentClient,
crudClient,
};
};

describe('useCreateContentMutation', () => {
test('should call rpcClient.create with input and resolve with output', async () => {
const { Wrapper, crudClient } = setup();
const input: CreateIn = { contentType: 'testType', data: { foo: 'bar' } };
const output = { test: 'test' };
crudClient.create.mockResolvedValueOnce(output);
Expand All @@ -39,3 +47,33 @@ describe('useCreateContentMutation', () => {
expect(result.current.data).toEqual(output);
});
});

describe('useUpdateContentMutation', () => {
test('should call rpcClient.update with input and resolve with output', async () => {
const { Wrapper, crudClient } = setup();
const input: UpdateIn = { contentType: 'testType', data: { foo: 'bar' } };
const output = { test: 'test' };
crudClient.update.mockResolvedValueOnce(output);
const { result, waitFor } = renderHook(() => useUpdateContentMutation(), { wrapper: Wrapper });
result.current.mutate(input);

await waitFor(() => result.current.isSuccess);

expect(result.current.data).toEqual(output);
});
});

describe('useDeleteContentMutation', () => {
test('should call rpcClient.delete with input and resolve with output', async () => {
const { Wrapper, crudClient } = setup();
const input: DeleteIn = { contentType: 'testType', data: { foo: 'bar' } };
const output = { test: 'test' };
crudClient.delete.mockResolvedValueOnce(output);
const { result, waitFor } = renderHook(() => useDeleteContentMutation(), { wrapper: Wrapper });
result.current.mutate(input);

await waitFor(() => result.current.isSuccess);

expect(result.current.data).toEqual(output);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import { useMutation } from '@tanstack/react-query';
import { useContentClient } from './content_client_context';
import type { CreateIn } from '../../common';
import type { CreateIn, UpdateIn, DeleteIn } from '../../common';

export const useCreateContentMutation = <I extends CreateIn = CreateIn, O = unknown>() => {
const contentClient = useContentClient();
Expand All @@ -18,3 +18,21 @@ export const useCreateContentMutation = <I extends CreateIn = CreateIn, O = unkn
},
});
};

export const useUpdateContentMutation = <I extends UpdateIn = UpdateIn, O = unknown>() => {
Copy link
Copy Markdown
Contributor

@sebelga sebelga Feb 15, 2023

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If that works we can apply the same for queries

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

const contentClient = useContentClient();
return useMutation({
mutationFn: (input: I) => {
return contentClient.update(input);
},
});
};

export const useDeleteContentMutation = <I extends DeleteIn = DeleteIn, O = unknown>() => {
const contentClient = useContentClient();
return useMutation({
mutationFn: (input: I) => {
return contentClient.delete(input);
},
});
};
Loading