From d0ceac99877f5e6723dd07068737c308a895b103 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 13 Mar 2023 16:23:11 -0500 Subject: [PATCH 1/6] Pass zodSchema directly to entity hooks for safeParse validation --- app/soapbox/entity-store/hooks/useEntities.ts | 18 ++++++++------- app/soapbox/entity-store/hooks/useEntity.ts | 15 ++++++++----- app/soapbox/hooks/useGroups.ts | 22 ++++--------------- 3 files changed, 24 insertions(+), 31 deletions(-) diff --git a/app/soapbox/entity-store/hooks/useEntities.ts b/app/soapbox/entity-store/hooks/useEntities.ts index 909273ea3..fa7e26d62 100644 --- a/app/soapbox/entity-store/hooks/useEntities.ts +++ b/app/soapbox/entity-store/hooks/useEntities.ts @@ -1,4 +1,5 @@ import { useEffect } from 'react'; +import z from 'zod'; import { getNextLink, getPrevLink } from 'soapbox/api'; import { useApi, useAppDispatch, useAppSelector } from 'soapbox/hooks'; @@ -17,9 +18,9 @@ type EntityPath = [ ] /** Additional options for the hook. */ -interface UseEntitiesOpts { - /** A parser function that returns the desired type, or undefined if validation fails. */ - parser?: (entity: unknown) => TEntity | undefined +interface UseEntitiesOpts { + /** A zod schema to parse the API entities. */ + schema?: z.ZodType /** * Time (milliseconds) until this query becomes stale and should be refetched. * It is 1 minute by default, and can be set to `Infinity` to opt-out of automatic fetching. @@ -41,8 +42,8 @@ function useEntities( const [entityType, listKey] = path; - const defaultParser = (entity: unknown) => entity as TEntity; - const parseEntity = opts.parser || defaultParser; + const defaultSchema = z.custom(); + const schema = opts.schema || defaultSchema; const cache = useAppSelector(state => state.entities[entityType]); const list = cache?.lists[listKey]; @@ -51,9 +52,10 @@ function useEntities( const entities: readonly TEntity[] = entityIds ? ( Array.from(entityIds).reduce((result, id) => { - const entity = parseEntity(cache?.store[id] as unknown); - if (entity) { - result.push(entity); + // TODO: parse after fetch, not during render. + const entity = schema.safeParse(cache?.store[id]); + if (entity.success) { + result.push(entity.data); } return result; }, []) diff --git a/app/soapbox/entity-store/hooks/useEntity.ts b/app/soapbox/entity-store/hooks/useEntity.ts index d0bae8630..d242c9b4e 100644 --- a/app/soapbox/entity-store/hooks/useEntity.ts +++ b/app/soapbox/entity-store/hooks/useEntity.ts @@ -1,4 +1,5 @@ import { useEffect, useState } from 'react'; +import z from 'zod'; import { useApi, useAppDispatch, useAppSelector } from 'soapbox/hooks'; @@ -10,8 +11,8 @@ type EntityPath = [entityType: string, entityId: string] /** Additional options for the hook. */ interface UseEntityOpts { - /** A parser function that returns the desired type, or undefined if validation fails. */ - parser?: (entity: unknown) => TEntity | undefined + /** A zod schema to parse the API entity. */ + schema?: z.ZodType /** Whether to refetch this entity every time the hook mounts, even if it's already in the store. */ refetch?: boolean } @@ -26,10 +27,14 @@ function useEntity( const [entityType, entityId] = path; - const defaultParser = (entity: unknown) => entity as TEntity; - const parseEntity = opts.parser || defaultParser; + const defaultSchema = z.custom(); + const schema = opts.schema || defaultSchema; - const entity = useAppSelector(state => parseEntity(state.entities[entityType]?.store[entityId])); + const entity = useAppSelector(state => { + // TODO: parse after fetch, not during render. + const result = schema.safeParse(state.entities[entityType]?.store[entityId]); + return result.success ? result.data : undefined; + }); const [isFetching, setIsFetching] = useState(false); const isLoading = isFetching && !entity; diff --git a/app/soapbox/hooks/useGroups.ts b/app/soapbox/hooks/useGroups.ts index 65437675b..7873c4f5d 100644 --- a/app/soapbox/hooks/useGroups.ts +++ b/app/soapbox/hooks/useGroups.ts @@ -3,7 +3,7 @@ import { groupSchema, Group } from 'soapbox/schemas/group'; import { groupRelationshipSchema, GroupRelationship } from 'soapbox/schemas/group-relationship'; function useGroups() { - const { entities, ...result } = useEntities(['Group', ''], '/api/v1/groups', { parser: parseGroup }); + const { entities, ...result } = useEntities(['Group', ''], '/api/v1/groups', { schema: groupSchema }); const { relationships } = useGroupRelationships(entities.map(entity => entity.id)); const groups = entities.map((group) => ({ ...group, relationship: relationships[group.id] || null })); @@ -15,7 +15,7 @@ function useGroups() { } function useGroup(groupId: string, refetch = true) { - const { entity: group, ...result } = useEntity(['Group', groupId], `/api/v1/groups/${groupId}`, { parser: parseGroup, refetch }); + const { entity: group, ...result } = useEntity(['Group', groupId], `/api/v1/groups/${groupId}`, { schema: groupSchema, refetch }); const { entity: relationship } = useGroupRelationship(groupId); return { @@ -25,13 +25,13 @@ function useGroup(groupId: string, refetch = true) { } function useGroupRelationship(groupId: string) { - return useEntity(['GroupRelationship', groupId], `/api/v1/groups/relationships?id[]=${groupId}`, { parser: parseGroupRelationship }); + return useEntity(['GroupRelationship', groupId], `/api/v1/groups/relationships?id[]=${groupId}`, { schema: groupRelationshipSchema }); } function useGroupRelationships(groupIds: string[]) { const q = groupIds.map(id => `id[]=${id}`).join('&'); const endpoint = groupIds.length ? `/api/v1/groups/relationships?${q}` : undefined; - const { entities, ...result } = useEntities(['GroupRelationship', q], endpoint, { parser: parseGroupRelationship }); + const { entities, ...result } = useEntities(['GroupRelationship', q], endpoint, { schema: groupRelationshipSchema }); const relationships = entities.reduce>((map, relationship) => { map[relationship.id] = relationship; @@ -44,18 +44,4 @@ function useGroupRelationships(groupIds: string[]) { }; } -const parseGroup = (entity: unknown) => { - const result = groupSchema.safeParse(entity); - if (result.success) { - return result.data; - } -}; - -const parseGroupRelationship = (entity: unknown) => { - const result = groupRelationshipSchema.safeParse(entity); - if (result.success) { - return result.data; - } -}; - export { useGroup, useGroups }; \ No newline at end of file From e9ae8d2c453d3753730b983a980816816ebfb134 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 13 Mar 2023 16:37:40 -0500 Subject: [PATCH 2/6] Fix filteredArray logic --- app/soapbox/schemas/utils.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/soapbox/schemas/utils.ts b/app/soapbox/schemas/utils.ts index d0bc4cc8f..72f5f49d9 100644 --- a/app/soapbox/schemas/utils.ts +++ b/app/soapbox/schemas/utils.ts @@ -4,10 +4,13 @@ import type { CustomEmoji } from './custom-emoji'; /** Validates individual items in an array, dropping any that aren't valid. */ function filteredArray(schema: T) { - return z.any().array().transform((arr) => ( - arr.map((item) => schema.safeParse(item).success ? item as z.infer : undefined) - .filter((item): item is z.infer => Boolean(item)) - )); + return z.any().array() + .transform((arr) => ( + arr.map((item) => { + const parsed = schema.safeParse(item); + return parsed.success ? parsed.data : undefined; + }).filter((item): item is z.infer => Boolean(item)) + )); } /** Map a list of CustomEmoji to their shortcodes. */ From a19b1e83a9a7e9ee426813cbcb8f11e4c3d892d0 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 13 Mar 2023 16:39:23 -0500 Subject: [PATCH 3/6] EntityStore: parse entities after fetch, not during render (performance) --- app/soapbox/entity-store/hooks/useEntities.ts | 12 +++++++----- app/soapbox/entity-store/hooks/useEntity.ts | 9 +++------ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/soapbox/entity-store/hooks/useEntities.ts b/app/soapbox/entity-store/hooks/useEntities.ts index fa7e26d62..72cec62f5 100644 --- a/app/soapbox/entity-store/hooks/useEntities.ts +++ b/app/soapbox/entity-store/hooks/useEntities.ts @@ -3,6 +3,7 @@ import z from 'zod'; import { getNextLink, getPrevLink } from 'soapbox/api'; import { useApi, useAppDispatch, useAppSelector } from 'soapbox/hooks'; +import { filteredArray } from 'soapbox/schemas/utils'; import { entitiesFetchFail, entitiesFetchRequest, entitiesFetchSuccess } from '../actions'; @@ -52,10 +53,9 @@ function useEntities( const entities: readonly TEntity[] = entityIds ? ( Array.from(entityIds).reduce((result, id) => { - // TODO: parse after fetch, not during render. - const entity = schema.safeParse(cache?.store[id]); - if (entity.success) { - result.push(entity.data); + const entity = cache?.store[id]; + if (entity) { + result.push(entity as TEntity); } return result; }, []) @@ -74,7 +74,9 @@ function useEntities( dispatch(entitiesFetchRequest(entityType, listKey)); try { const response = await api.get(url); - dispatch(entitiesFetchSuccess(response.data, entityType, listKey, { + const entities = filteredArray(schema).parse(response.data); + + dispatch(entitiesFetchSuccess(entities, entityType, listKey, { next: getNextLink(response), prev: getPrevLink(response), fetching: false, diff --git a/app/soapbox/entity-store/hooks/useEntity.ts b/app/soapbox/entity-store/hooks/useEntity.ts index d242c9b4e..92f20560e 100644 --- a/app/soapbox/entity-store/hooks/useEntity.ts +++ b/app/soapbox/entity-store/hooks/useEntity.ts @@ -30,11 +30,7 @@ function useEntity( const defaultSchema = z.custom(); const schema = opts.schema || defaultSchema; - const entity = useAppSelector(state => { - // TODO: parse after fetch, not during render. - const result = schema.safeParse(state.entities[entityType]?.store[entityId]); - return result.success ? result.data : undefined; - }); + const entity = useAppSelector(state => state.entities[entityType]?.store[entityId] as TEntity | undefined); const [isFetching, setIsFetching] = useState(false); const isLoading = isFetching && !entity; @@ -42,7 +38,8 @@ function useEntity( const fetchEntity = () => { setIsFetching(true); api.get(endpoint).then(({ data }) => { - dispatch(importEntities([data], entityType)); + const entity = schema.parse(data); + dispatch(importEntities([entity], entityType)); setIsFetching(false); }).catch(() => { setIsFetching(false); From 8547aeb5175c21441bc24b4b93b455c1e1ba05d5 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 13 Mar 2023 17:45:35 -0500 Subject: [PATCH 4/6] Add useGetState hook --- app/soapbox/hooks/index.ts | 1 + app/soapbox/hooks/useApi.ts | 9 +++------ app/soapbox/hooks/useGetState.ts | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 6 deletions(-) create mode 100644 app/soapbox/hooks/useGetState.ts diff --git a/app/soapbox/hooks/index.ts b/app/soapbox/hooks/index.ts index 1b0545e83..9cfd0a5e1 100644 --- a/app/soapbox/hooks/index.ts +++ b/app/soapbox/hooks/index.ts @@ -5,6 +5,7 @@ export { useAppSelector } from './useAppSelector'; export { useClickOutside } from './useClickOutside'; export { useCompose } from './useCompose'; export { useDebounce } from './useDebounce'; +export { useGetState } from './useGetState'; export { useGroup, useGroups } from './useGroups'; export { useGroupsPath } from './useGroupsPath'; export { useDimensions } from './useDimensions'; diff --git a/app/soapbox/hooks/useApi.ts b/app/soapbox/hooks/useApi.ts index 1d98a6166..2e22df997 100644 --- a/app/soapbox/hooks/useApi.ts +++ b/app/soapbox/hooks/useApi.ts @@ -1,12 +1,9 @@ import api from 'soapbox/api'; -import { useAppDispatch } from './useAppDispatch'; +import { useGetState } from './useGetState'; /** Use stateful Axios client with auth from Redux. */ export const useApi = () => { - const dispatch = useAppDispatch(); - - return dispatch((_dispatch, getState) => { - return api(getState); - }); + const getState = useGetState(); + return api(getState); }; diff --git a/app/soapbox/hooks/useGetState.ts b/app/soapbox/hooks/useGetState.ts new file mode 100644 index 000000000..848c0c348 --- /dev/null +++ b/app/soapbox/hooks/useGetState.ts @@ -0,0 +1,14 @@ +import { useAppDispatch } from './useAppDispatch'; + +import type { RootState } from 'soapbox/store'; + +/** + * Provides a `getState()` function to hooks. + * You should prefer `useAppSelector` when possible. + */ +function useGetState() { + const dispatch = useAppDispatch(); + return () => dispatch((_, getState: () => RootState) => getState()); +} + +export { useGetState }; \ No newline at end of file From b93a299009ae9de52c9525857922112661ed389d Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 13 Mar 2023 17:53:54 -0500 Subject: [PATCH 5/6] useEntities(): refactor into smaller performant selectors and hooks --- app/soapbox/entity-store/hooks/useEntities.ts | 80 +++++++++++-------- 1 file changed, 48 insertions(+), 32 deletions(-) diff --git a/app/soapbox/entity-store/hooks/useEntities.ts b/app/soapbox/entity-store/hooks/useEntities.ts index 72cec62f5..1d1cd5eed 100644 --- a/app/soapbox/entity-store/hooks/useEntities.ts +++ b/app/soapbox/entity-store/hooks/useEntities.ts @@ -2,12 +2,12 @@ import { useEffect } from 'react'; import z from 'zod'; import { getNextLink, getPrevLink } from 'soapbox/api'; -import { useApi, useAppDispatch, useAppSelector } from 'soapbox/hooks'; +import { useApi, useAppDispatch, useAppSelector, useGetState } from 'soapbox/hooks'; import { filteredArray } from 'soapbox/schemas/utils'; import { entitiesFetchFail, entitiesFetchRequest, entitiesFetchSuccess } from '../actions'; -import type { Entity } from '../types'; +import type { Entity, EntityListState } from '../types'; import type { RootState } from 'soapbox/store'; /** Tells us where to find/store the entity in the cache. */ @@ -40,40 +40,26 @@ function useEntities( ) { const api = useApi(); const dispatch = useAppDispatch(); + const getState = useGetState(); const [entityType, listKey] = path; + const entities = useAppSelector(state => selectEntities(state, path)); - const defaultSchema = z.custom(); - const schema = opts.schema || defaultSchema; + const isFetching = useListState(path, 'fetching'); + const lastFetchedAt = useListState(path, 'lastFetchedAt'); - const cache = useAppSelector(state => state.entities[entityType]); - const list = cache?.lists[listKey]; - - const entityIds = list?.ids; - - const entities: readonly TEntity[] = entityIds ? ( - Array.from(entityIds).reduce((result, id) => { - const entity = cache?.store[id]; - if (entity) { - result.push(entity as TEntity); - } - return result; - }, []) - ) : []; - - const isFetching = Boolean(list?.state.fetching); - const isLoading = isFetching && entities.length === 0; - const hasNextPage = Boolean(list?.state.next); - const hasPreviousPage = Boolean(list?.state.prev); + const next = useListState(path, 'next'); + const prev = useListState(path, 'prev'); const fetchPage = async(url: string): Promise => { // Get `isFetching` state from the store again to prevent race conditions. - const isFetching = dispatch((_, getState: () => RootState) => Boolean(getState().entities[entityType]?.lists[listKey]?.state.fetching)); + const isFetching = selectListState(getState(), path, 'fetching'); if (isFetching) return; dispatch(entitiesFetchRequest(entityType, listKey)); try { const response = await api.get(url); + const schema = opts.schema || z.custom(); const entities = filteredArray(schema).parse(response.data); dispatch(entitiesFetchSuccess(entities, entityType, listKey, { @@ -95,23 +81,18 @@ function useEntities( }; const fetchNextPage = async(): Promise => { - const next = list?.state.next; - if (next) { await fetchPage(next); } }; const fetchPreviousPage = async(): Promise => { - const prev = list?.state.prev; - if (prev) { await fetchPage(prev); } }; const staleTime = opts.staleTime ?? 60000; - const lastFetchedAt = list?.state.lastFetchedAt; useEffect(() => { if (!isFetching && (!lastFetchedAt || lastFetchedAt.getTime() + staleTime <= Date.now())) { @@ -123,14 +104,49 @@ function useEntities( entities, fetchEntities, isFetching, - isLoading, - hasNextPage, - hasPreviousPage, + isLoading: isFetching && entities.length === 0, + hasNextPage: !!next, + hasPreviousPage: !!prev, fetchNextPage, fetchPreviousPage, }; } +/** Get cache at path from Redux. */ +const selectCache = (state: RootState, path: EntityPath) => state.entities[path[0]]; + +/** Get list at path from Redux. */ +const selectList = (state: RootState, path: EntityPath) => selectCache(state, path)?.lists[path[1]]; + +/** Select a particular item from a list state. */ +function selectListState(state: RootState, path: EntityPath, key: K) { + const listState = selectList(state, path)?.state; + return listState ? listState[key] : undefined; +} + +/** Hook to get a particular item from a list state. */ +function useListState(path: EntityPath, key: K) { + return useAppSelector(state => selectListState(state, path, key)); +} + +/** Get list of entities from Redux. */ +function selectEntities(state: RootState, path: EntityPath): readonly TEntity[] { + const cache = selectCache(state, path); + const list = selectList(state, path); + + const entityIds = list?.ids; + + return entityIds ? ( + Array.from(entityIds).reduce((result, id) => { + const entity = cache?.store[id]; + if (entity) { + result.push(entity as TEntity); + } + return result; + }, []) + ) : []; +} + export { useEntities, }; \ No newline at end of file From 9df2bb4a863b69be2e69436de86f095bf3dd03c3 Mon Sep 17 00:00:00 2001 From: Alex Gleason Date: Mon, 13 Mar 2023 18:34:42 -0500 Subject: [PATCH 6/6] EntityStore: add tests for reducer, improve types, ensure error gets added to state --- .../entity-store/__tests__/reducer.test.ts | 79 +++++++++++++++++++ app/soapbox/entity-store/reducer.ts | 4 +- app/soapbox/entity-store/types.ts | 8 +- 3 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 app/soapbox/entity-store/__tests__/reducer.test.ts diff --git a/app/soapbox/entity-store/__tests__/reducer.test.ts b/app/soapbox/entity-store/__tests__/reducer.test.ts new file mode 100644 index 000000000..0da25f25e --- /dev/null +++ b/app/soapbox/entity-store/__tests__/reducer.test.ts @@ -0,0 +1,79 @@ +import { entitiesFetchFail, entitiesFetchRequest, importEntities } from '../actions'; +import reducer from '../reducer'; + +import type { EntityCache } from '../types'; + +interface TestEntity { + id: string + msg: string +} + +test('import entities', () => { + const entities: TestEntity[] = [ + { id: '1', msg: 'yolo' }, + { id: '2', msg: 'benis' }, + { id: '3', msg: 'boop' }, + ]; + + const action = importEntities(entities, 'TestEntity'); + const result = reducer(undefined, action); + const cache = result.TestEntity as EntityCache; + + expect(cache.store['1']!.msg).toBe('yolo'); + expect(Object.values(cache.lists).length).toBe(0); +}); + +test('import entities into a list', () => { + const entities: TestEntity[] = [ + { id: '1', msg: 'yolo' }, + { id: '2', msg: 'benis' }, + { id: '3', msg: 'boop' }, + ]; + + const action = importEntities(entities, 'TestEntity', 'thingies'); + const result = reducer(undefined, action); + const cache = result.TestEntity as EntityCache; + + expect(cache.store['2']!.msg).toBe('benis'); + expect(cache.lists.thingies?.ids.size).toBe(3); + + // Now try adding an additional item. + const entities2: TestEntity[] = [ + { id: '4', msg: 'hehe' }, + ]; + + const action2 = importEntities(entities2, 'TestEntity', 'thingies'); + const result2 = reducer(result, action2); + const cache2 = result2.TestEntity as EntityCache; + + expect(cache2.store['4']!.msg).toBe('hehe'); + expect(cache2.lists.thingies?.ids.size).toBe(4); + + // Finally, update an item. + const entities3: TestEntity[] = [ + { id: '2', msg: 'yolofam' }, + ]; + + const action3 = importEntities(entities3, 'TestEntity', 'thingies'); + const result3 = reducer(result2, action3); + const cache3 = result3.TestEntity as EntityCache; + + expect(cache3.store['2']!.msg).toBe('yolofam'); + expect(cache3.lists.thingies?.ids.size).toBe(4); // unchanged +}); + +test('fetching updates the list state', () => { + const action = entitiesFetchRequest('TestEntity', 'thingies'); + const result = reducer(undefined, action); + + expect(result.TestEntity!.lists.thingies!.state.fetching).toBe(true); +}); + +test('failure adds the error to the state', () => { + const error = new Error('whoopsie'); + + const action = entitiesFetchFail('TestEntity', 'thingies', error); + const result = reducer(undefined, action); + + expect(result.TestEntity!.lists.thingies!.state.error).toBe(error); +}); \ No newline at end of file diff --git a/app/soapbox/entity-store/reducer.ts b/app/soapbox/entity-store/reducer.ts index f2af680a1..bb1dcdd1d 100644 --- a/app/soapbox/entity-store/reducer.ts +++ b/app/soapbox/entity-store/reducer.ts @@ -48,6 +48,7 @@ const setFetching = ( entityType: string, listKey: string | undefined, isFetching: boolean, + error?: any, ) => { return produce(state, draft => { const cache = draft[entityType] ?? createCache(); @@ -55,6 +56,7 @@ const setFetching = ( if (typeof listKey === 'string') { const list = cache.lists[listKey] ?? createList(); list.state.fetching = isFetching; + list.state.error = error; cache.lists[listKey] = list; } @@ -72,7 +74,7 @@ function reducer(state: Readonly = {}, action: EntityAction): State { case ENTITIES_FETCH_REQUEST: return setFetching(state, action.entityType, action.listKey, true); case ENTITIES_FETCH_FAIL: - return setFetching(state, action.entityType, action.listKey, false); + return setFetching(state, action.entityType, action.listKey, false, action.error); default: return state; } diff --git a/app/soapbox/entity-store/types.ts b/app/soapbox/entity-store/types.ts index efec97df1..eb2a306a0 100644 --- a/app/soapbox/entity-store/types.ts +++ b/app/soapbox/entity-store/types.ts @@ -5,8 +5,8 @@ interface Entity { } /** Store of entities by ID. */ -interface EntityStore { - [id: string]: Entity | undefined +interface EntityStore { + [id: string]: TEntity | undefined } /** List of entity IDs and fetch state. */ @@ -32,9 +32,9 @@ interface EntityListState { } /** Cache data pertaining to a paritcular entity type.. */ -interface EntityCache { +interface EntityCache { /** Map of entities of this type. */ - store: EntityStore + store: EntityStore /** Lists of entity IDs for a particular purpose. */ lists: { [listKey: string]: EntityList | undefined