-
Notifications
You must be signed in to change notification settings - Fork 3
Description
Basically extracting a variety of potential tasks based on feedback in #1038. Just going to consolidate these here for now and we can create separate items later if needed.
1. Avoid usage of hardcoded/vendored query keys.
Due to missing functionality in @comapeo/core-react
, we still have some reliance on knowing the query keys used by it in order to do proper invalidation of relevant read hooks. Basically anything that references ROOT_QUERY_KEY
. As of this writing, some hooks related to maps, invites, and sync are affected by this.
The ideal solution is to update @comapeo/core
and/or @comapeo/core-react
to provide functionality such that manual invalidation in the mobile app isn't necessary.
2. Simplify ActiveProjectContext.tsx
.
Before the core react integration, the implemented provider ensured that there was an active project id and project api instance for the rest of the app to work with. the usage of this generally looked like this:
const {projectId, projectApi} = useActiveProject();
After the major integration work, it's still technically doing the same thing but there's no need for it to hold+pass a project api instance, as that should is handled by using useSingleProject()
throughout the app. now the consumption of that context looks like this:
// 🟠 no more need for the projectApi field
const {projectId} = useActiveProject()
const {data: projectApi} = useSingleProject({projectId})
We still need the context for guaranteeing an active project id and making that available for the rest of the app. I'm proposing that the context be reduced to the following responsibilities:
- conditionally rendering until an active project id has been identified/selected
- pass the value from (1) down to the rest of the app.
The ideal usage of this context looks like the following for any component that deals with project-specific APIs:
const activeProjectId = useActiveProjectId()
const {data: projectApi} = useSingleProject({projectId: activeProjectId})
3. Reducing the number of "wrapper" hooks where appropriate
Note: this one could be viewed as more stylistic so could be less urgent to address
We have a variety of custom hooks that essentially wrap the hooks from core-react for the sake of being more opinionated, having a clearer name, or not having to repeat options. In some cases, this is useful because there might be additional logic that happens that would be tedious or error-prone to copy over with each usage. However, we have a few cases where we wrap hooks from core-react that don't do much aside from adding the convenience of not having to know how to get the relevant project id. For example, I find the following to be unnecessary as currently implemented:
comapeo-mobile/src/frontend/hooks/server/projects.ts
Lines 21 to 29 in 0af0bfa
export function useProjectSettings() { | |
const {projectId} = useActiveProject(); | |
return useComapeoProjectSettings({projectId}); | |
} | |
export function useGetOwnRole() { | |
const {projectId} = useActiveProject(); | |
return useOwnRoleInProject({projectId}); | |
} |
The suggested approach would be to use the core-react hooks directly where they're needed. For example:
import {useProjectSettings, useOwnRoleInProject} from '@comapeo/core-react'
function ExampleScreen() {
const projectId = useActiveProjectId()
const {data: projectSettings} = useProjectSettings({projectId})
const {data: ownRole} = useOwnRoleInProject({projectId})
}
The main advantage I see from this is that it's extremely clear where the projectId
that's being used comes from. As of right now, we access it via a context, but there's a possibility that it could come from something like a route param, in which case the above example becomes:
import {useProjectSettings, useOwnRoleInProject} from '@comapeo/core-react'
function ExampleScreen({route}) {
// 🟠 Only this changes. The hooks using it stay the same!
const {projectId} = route.params
const {data: projectSettings} = useProjectSettings({projectId})
const {data: ownRole} = useOwnRoleInProject({projectId})
}
By creating unnecessary wrapper hooks, you lose that visibility and flexibility. The wrapper hook becomes less agnostic about the source of the project id in this case, meaning that there's more to change if that source needs to change. You can get around that a bit by having the wrapper hook accept an argument e.g.
export function useGetOwnRole(projectId: string) {
return useOwnRoleInProject({projectId});
}
but then it becomes clearer that this wrapper is unnecessary as it's essentially just a renaming of the original core-react hook.
as mentioned before, there are cases where having the wrapper hooks make sense. for example, if you want to have specific custom logic or options that are provided to the underlying core-react hook. for example:
comapeo-mobile/src/frontend/hooks/server/projects.ts
Lines 31 to 44 in 0af0bfa
export function useGetRemoteArchives() { | |
const {projectId} = useActiveProject(); | |
const {data: members, error, isRefetching} = useManyMembers({projectId}); | |
const archives = useMemo(() => { | |
return members?.filter(m => m.deviceType === 'selfHostedServer') ?? []; | |
}, [members]); | |
return { | |
data: archives, | |
error, | |
isRefetching, | |
}; | |
} |
in this case, I would suggest making the projectId
an argument for this hook e.g.
export function useGetRemoteArchives(projectId: string) {
const {data: members, error, isRefetching} = useManyMembers({projectId});
const archives = useMemo(() => {
return members?.filter(m => m.deviceType === 'selfHostedServer') ?? [];
}, [members]);
return {
data: archives,
error,
isRefetching,
};
}
this preserves the benefits of having custom logic or options but also introduces the benefit of making the source of the projectId more flexible, as the usage then becomes something like this:
function ExampleScreen() {
// 🟠 Again, this could come from something like route params
const projectId = useActiveProject()
const remoteArchives = useGetRemoteArchives(projectId)
}
4. Cleaning up some of the media-related custom hooks.
Some of the hooks related to creating/reading attachments (or urls to them) can use some significant cleanup. The major core-react integration work potentially changes some of the runtime behavior (example)
5. (Most likely) a small regression in granular loading state of observation thumbnails
Mostly detailed in this comment but it's a symptom of using suspense-based approaches for loading states. the existing components that are affected by this specific case are a bit overcomplicated from what I can tell, to the point where updating them to account for the new core react hooks wouldn't be a trivial amount of work.