Skip to content

Commit

Permalink
Added tests to AdminX framework package (TryGhost#19022)
Browse files Browse the repository at this point in the history
refs https://github.com/TryGhost/Product/issues/4159

---

<!-- Leave the line below if you'd like GitHub Copilot to generate a
summary from your commit -->
<!--
copilot:summary
-->
### <samp>🤖[[deprecated]](https://githubnext.com/copilot-for-prs-sunset)
Generated by Copilot at 9e68f4d</samp>

This pull request refactors several components in the `admin-x-settings`
app to use common hooks from the `@tryghost/admin-x-framework` package,
which reduces code duplication and improves consistency. It also updates
the `package.json` file and adds unit tests for the `admin-x-framework`
package, which improves the formatting, testing, and dependency
management. Additionally, it makes some minor changes to the `hooks.ts`,
`FrameworkProvider.tsx`, and `.eslintrc.cjs` files in the
`admin-x-framework` package, which enhance the public API and the
linting configuration.
  • Loading branch information
binary-koan authored Nov 20, 2023
1 parent b1666f5 commit 5e057de
Show file tree
Hide file tree
Showing 37 changed files with 874 additions and 157 deletions.
41 changes: 25 additions & 16 deletions apps/admin-x-framework/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,54 7,57 @@
"private": true,
"exports": {
".": {
"import": "./es/index.js",
"types": "./types/index.d.ts"
"import": "./es/index.js",
"types": "./types/index.d.ts"
},
"./errors": {
"import": "./es/errors.js",
"types": "./types/errors.d.ts"
"import": "./es/errors.js",
"types": "./types/errors.d.ts"
},
"./helpers": {
"import": "./es/helpers.js",
"types": "./types/helpers.d.ts"
"import": "./es/helpers.js",
"types": "./types/helpers.d.ts"
},
"./hooks": {
"import": "./es/hooks.js",
"types": "./types/hooks.d.ts"
"import": "./es/hooks.js",
"types": "./types/hooks.d.ts"
},
"./routing": {
"import": "./es/routing.js",
"types": "./types/routing.d.ts"
"import": "./es/routing.js",
"types": "./types/routing.d.ts"
},
"./api/*": {
"import": "./es/api/*.js",
"types": "./types/api/*.d.ts"
"import": "./es/api/*.js",
"types": "./types/api/*.d.ts"
}
},
"sideEffects": false,
"scripts": {
"build": "vite build && tsc -p tsconfig.declaration.json",
"prepare": "yarn build",
"test": "yarn test:types",
"test": "yarn test:types && yarn test:unit",
"test:types": "tsc --noEmit",
"test:unit": "vitest run --coverage",
"lint:code": "eslint --ext .js,.ts,.cjs,.tsx src/ --cache",
"lint": "yarn lint:code && (yarn lint:test || echo \"TODO ADD TESTS TO LINT\")",
"lint": "yarn lint:code && yarn lint:test",
"lint:test": "eslint -c test/.eslintrc.cjs --ext .js,.ts,.cjs,.tsx test/ --cache"
},
"files": [
"es",
"types"
],
"devDependencies": {
"@testing-library/react": "14.1.0",
"@types/mocha": "10.0.1",
"@vitejs/plugin-react": "4.2.0",
"c8": "8.0.1",
"eslint-plugin-react-hooks": "4.6.0",
"eslint-plugin-react-refresh": "0.4.3",
"mocha": "10.2.0",
"react": "18.2.0",
"react-dom": "18.2.0",
"sinon": "17.0.0",
"ts-node": "10.9.1",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"typescript": "5.2.2",
"vite": "4.5.0"
},
Expand All @@ -74,6 77,12 @@
"build",
{"projects": ["@tryghost/admin-x-design-system"], "target": "build"}
]
},
"test:unit": {
"dependsOn": [
"test:unit",
{"projects": ["@tryghost/admin-x-design-system"], "target": "build"}
]
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion apps/admin-x-framework/src/api/offers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 49,7 @@ export const useBrowseOffers = createQuery<OffersResponseType>({

export const useBrowseOffersById = createQueryWithId<OffersResponseType>({
dataType,
path: `/offers/`
path: id => `/offers/${id}/`
});

export const useEditOffer = createMutation<OfferEditResponseType, Offer>({
Expand Down
2 changes: 2 additions & 0 deletions apps/admin-x-framework/src/hooks.ts
Original file line number Diff line number Diff line change
@@ -1,4 1,6 @@
export {default as useFilterableApi} from './hooks/useFilterableApi';
export {default as useForm} from './hooks/useForm';
export type {Dirtyable, ErrorMessages, FormHook, OkProps, SaveHandler, SaveState} from './hooks/useForm';
export {default as useHandleError} from './hooks/useHandleError';
export {usePermission} from './hooks/usePermissions';

File renamed without changes.
2 changes: 1 addition & 1 deletion apps/admin-x-framework/src/providers/FrameworkProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 8,7 @@ export interface FrameworkProviderProps {
basePath: string;
ghostVersion: string;
externalNavigate: RoutingProviderProps['externalNavigate'];
modals: RoutingProviderProps['modals'];
modals?: RoutingProviderProps['modals'];
unsplashConfig: {
Authorization: string;
'Accept-Version': string;
Expand Down
5 changes: 2 additions & 3 deletions apps/admin-x-framework/src/utils/api/fetchApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 19,7 @@ export const useFetchApi = () => {
const {ghostVersion, sentryDSN} = useFramework();

// eslint-disable-next-line @typescript-eslint/no-explicit-any
return async <ResponseData = any>(endpoint: string | URL, options: RequestOptions = {}): Promise<ResponseData> => {
return async <ResponseData = any>(endpoint: string | URL, {headers = {}, retry, ...options}: RequestOptions = {}): Promise<ResponseData> => {
// By default, we set the Content-Type header to application/json
const defaultHeaders: Record<string, string> = {
'app-pragma': 'no-cache',
Expand All @@ -28,7 28,6 @@ export const useFetchApi = () => {
if (typeof options.body === 'string') {
defaultHeaders['content-type'] = 'application/json';
}
const headers = options?.headers || {};

const controller = new AbortController();
const {timeout} = options;
Expand All @@ -41,7 40,7 @@ export const useFetchApi = () => {
// 1. Server Unreachable error from the browser (code 0 or TypeError), typically from short internet blips
// 2. Maintenance error from Ghost, upgrade in progress so API is temporarily unavailable
let attempts = 0;
const shouldRetry = options.retry === true || options.retry === undefined;
const shouldRetry = retry !== false;
let retryingMs = 0;
const startTime = Date.now();
const maxRetryingMs = 15_000;
Expand Down
37 changes: 14 additions & 23 deletions apps/admin-x-framework/src/utils/api/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 17,6 @@ export interface Meta {
}
}

const parameterizedPath = (path: string, params: string | string[]) => {
const paramList = Array.isArray(params) ? params : [params];
return paramList.reduce(function (updatedPath, param) {
updatedPath = updatedPath param '/';
updatedPath.replace(/:[a-z0-9] /, encodeURIComponent(param));
return updatedPath;
}, path);
};

interface QueryOptions<ResponseData> {
dataType: string
path: string
Expand Down Expand Up @@ -147,8 138,8 @@ export const createInfiniteQuery = <ResponseData>(options: InfiniteQueryOptions<
};
};

export const createQueryWithId = <ResponseData>(options: QueryOptions<ResponseData>) => (id: string, {searchParams, ...query}: QueryHookOptions<ResponseData> = {}) => {
const queryHook = createQuery<ResponseData>({...options, path: parameterizedPath(options.path, id)});
export const createQueryWithId = <ResponseData>(options: Omit<QueryOptions<ResponseData>, 'path'> & {path: (id: string) => string}) => (id: string, {searchParams, ...query}: QueryHookOptions<ResponseData> = {}) => {
const queryHook = createQuery<ResponseData>({...options, path: options.path(id)});
return queryHook({searchParams: searchParams || options.defaultSearchParams, ...query});
};

Expand All @@ -165,7 156,7 @@ const mutate = <ResponseData, Payload>({fetchApi, path, payload, searchParams, o
path: string;
payload?: Payload;
searchParams?: Record<string, string>;
options: MutationOptions<ResponseData, Payload>
options: Omit<MutationOptions<ResponseData, Payload>, 'path'>
}) => {
const {defaultSearchParams, body, ...requestOptions} = options;
const url = apiUrl(path, searchParams || defaultSearchParams);
Expand All @@ -184,33 175,33 @@ const mutate = <ResponseData, Payload>({fetchApi, path, payload, searchParams, o
});
};

export const createMutation = <ResponseData, Payload>(options: MutationOptions<ResponseData, Payload>) => () => {
export const createMutation = <ResponseData, Payload>({path, searchParams, defaultSearchParams, updateQueries, invalidateQueries, ...mutateOptions}: MutationOptions<ResponseData, Payload>) => () => {
const fetchApi = useFetchApi();
const queryClient = useQueryClient();
const {onUpdate, onInvalidate, onDelete} = useFramework();

const afterMutate = useCallback((newData: ResponseData, payload: Payload) => {
if (options.invalidateQueries) {
queryClient.invalidateQueries([options.invalidateQueries.dataType]);
onInvalidate(options.invalidateQueries.dataType);
if (invalidateQueries) {
queryClient.invalidateQueries([invalidateQueries.dataType]);
onInvalidate(invalidateQueries.dataType);
}

if (options.updateQueries) {
queryClient.setQueriesData([options.updateQueries.dataType], (data: unknown) => options.updateQueries!.update(newData, data, payload));
if (options.updateQueries.emberUpdateType === 'createOrUpdate') {
onUpdate(options.updateQueries.dataType, newData);
} else if (options.updateQueries.emberUpdateType === 'delete') {
if (updateQueries) {
queryClient.setQueriesData([updateQueries.dataType], (data: unknown) => updateQueries!.update(newData, data, payload));
if (updateQueries.emberUpdateType === 'createOrUpdate') {
onUpdate(updateQueries.dataType, newData);
} else if (updateQueries.emberUpdateType === 'delete') {
if (typeof payload !== 'string') {
throw new Error('Expected delete mutation to have a string (ID) payload. Either change the payload or update the createMutation hook');
}

onDelete(options.updateQueries.dataType, payload);
onDelete(updateQueries.dataType, payload);
}
}
}, [onInvalidate, onUpdate, onDelete, queryClient]);

return useMutation<ResponseData, unknown, Payload>({
mutationFn: payload => mutate({fetchApi, path: options.path(payload), payload, searchParams: options.searchParams?.(payload) || options.defaultSearchParams, options}),
mutationFn: payload => mutate({fetchApi, path: path(payload), payload, searchParams: searchParams?.(payload) || defaultSearchParams, options: mutateOptions}),
onSuccess: afterMutate
});
};
3 changes: 1 addition & 2 deletions apps/admin-x-framework/test/.eslintrc.cjs
Original file line number Diff line number Diff line change
@@ -1,7 1,6 @@
module.exports = {
parser: '@typescript-eslint/parser',
plugins: ['ghost'],
extends: [
'plugin:ghost/test'
'plugin:ghost/ts-test'
]
};
8 changes: 0 additions & 8 deletions apps/admin-x-framework/test/hello.test.ts

This file was deleted.

75 changes: 75 additions & 0 deletions apps/admin-x-framework/test/unit/hooks/useForm.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 1,75 @@
import {act, renderHook} from '@testing-library/react';
import * as assert from 'assert/strict';
import useForm from '../../../src/hooks/useForm';

describe('useForm', function () {
describe('formState', function () {
it('returns the initial form state', function () {
const {result} = renderHook(() => useForm({
initialState: {a: 1},
onSave: () => {}
}));

assert.deepEqual(result.current.formState, {a: 1});
});
});

describe('updateForm', function () {
it('updates the form state', function () {
const {result} = renderHook(() => useForm({
initialState: {a: 1},
onSave: () => {}
}));

act(() => result.current.updateForm(state => ({...state, b: 2})));

assert.deepEqual(result.current.formState, {a: 1, b: 2});
});

it('sets the saveState to unsaved', function () {
const {result} = renderHook(() => useForm({
initialState: {a: 1},
onSave: () => {}
}));

act(() => result.current.updateForm(state => ({...state, a: 2})));

assert.deepEqual(result.current.saveState, 'unsaved');
});
});

describe('handleSave', function () {
it('does nothing when the state has not changed', async function () {
let onSaveCalled = false;

const {result} = renderHook(() => useForm({
initialState: {a: 1},
onSave: () => {
onSaveCalled = true;
}
}));

assert.equal(await act(() => result.current.handleSave()), true);

assert.equal(result.current.saveState, '');
assert.equal(onSaveCalled, false);
});

it('calls the onSave callback when the state has changed', async function () {
let onSaveCalled = false;

const {result} = renderHook(() => useForm({
initialState: {a: 1},
onSave: () => {
onSaveCalled = true;
}
}));

act(() => result.current.updateForm(state => ({...state, a: 2})));
assert.equal(await act(() => result.current.handleSave()), true);

assert.equal(result.current.saveState, 'saved');
assert.equal(onSaveCalled, true);
});
});
});
58 changes: 58 additions & 0 deletions apps/admin-x-framework/test/unit/utils/api/fetchApi.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 1,58 @@
import {renderHook} from '@testing-library/react';
import React, {ReactNode} from 'react';
import FrameworkProvider from '../../../../src/providers/FrameworkProvider';
import {useFetchApi} from '../../../../src/utils/api/fetchApi';
import {withMockFetch} from '../../../utils/mockFetch';

const wrapper: React.FC<{ children: ReactNode }> = ({children}) => (
<FrameworkProvider
basePath=''
externalNavigate={() => {}}
ghostVersion='5.x'
sentryDSN=''
unsplashConfig={{
Authorization: '',
'Accept-Version': '',
'Content-Type': '',
'App-Pragma': '',
'X-Unsplash-Cache': true
}}
onDelete={() => {}}
onInvalidate={() => {}}
onUpdate={() => {}}
>
{children}
</FrameworkProvider>
);

describe('useFetchApi', function () {
it('makes an API request', async function () {
await withMockFetch({
json: {test: 1}
}, async (mock) => {
const {result} = renderHook(() => useFetchApi(), {wrapper});

const data = await result.current<{test: number}>('http://localhost:3000/ghost/api/admin/test/', {
method: 'POST',
body: 'test',
retry: false
});

expect(data).toEqual({test: 1});

expect(mock.calls.length).toBe(1);
expect(mock.calls[0]).toEqual(['http://localhost:3000/ghost/api/admin/test/', {
body: 'test',
credentials: 'include',
headers: {
'app-pragma': 'no-cache',
'x-ghost-version': '5.x',
'content-type': 'application/json'
},
method: 'POST',
mode: 'cors',
signal: expect.any(AbortSignal)
}]);
});
});
});
Loading

0 comments on commit 5e057de

Please sign in to comment.