Repository navigation
Conversation
uses the version that adds the mutation context (see https://github.com/TanStack/query/releases/tag/v5.89.0)
provides access to unified query API
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| queryClient.invalidateQueries({ | ||
| queryKey: getJoinRequestsQueryKey(), | ||
| // NOTE: Only want to invalidate the query for getting all join requests (no cascade) | ||
| exact: true, | ||
| }) | ||
|
|
||
| // NOTE: Directly update query for getting specific join request | ||
| queryClient.setQueryData<InviteLinkJoiner.JoinRequest>( | ||
| getJoinRequestsByIdQueryKey({ inviteId: update.inviteId }), | ||
| (prev) => { | ||
| if (!prev) { | ||
| return prev | ||
| } | ||
|
|
||
| return { ...prev, ...update } | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Part of me is a little bit hesitant on the implementation here. We invalidate the query used to get all of the join requests but we manually update the data for the query for affected join request. Usually I prefer to just call invalidateQueries with the appropriate cascade and call it a day to avoid issues with client vs server state misalignment, but I think it's a bit more complicated in this case...
Granular updating of the single join request query data accounts for core removing access to the join request once it's in a "completed" state (e.g. status is completed, failed, etc). Without granular updating, what happens is something like the following:
- Call
useSingleJoinRequest() completedevent emits and invalidate all queries related to join request- Query from (1) refetches from core, which throws a not found error, and its
datastill reflects an "in-progress" status e.g.statusisconnected. basically, the data from (1) will never be able to represent a "completed" join request in this case.
From an end-user perspective, this sequence requires more complicated workarounds to support UI where a join request's completed state needs to be represented. Sometimes this is manageable enough to let the consuming app to handle it via the event listener + useState, but based on discussions with Cindy I think it makes more sense do that work here instead and rely on the facilities provided by react query.
The potentially confusing bit with this implementation is that we don't attempt to keep a reference to the completed join request for the query that gets all join requests, and still just rely on what core returns in this case (which is a list of only pending ones). Based on our UI requirements, I don't think this is an issue, but the inconsistency between this vs a specific join request might cause confusion. I've also made sure that the query for the single join request is configured to not rely too heavily on the query cache and do refetches when appropriate, which should mitigate the misalignment of client vs server state for this hook.
One can argue that we should take this approach for the hooks related to the local invites API as well, which have similar limitations as the invite links API. Something to consider for follow-up based on how it works out here...
| // NOTE: Ensure garbage collection occurs immediately when all observers are unmounted | ||
| // so that newly active instances only use data from refetch. | ||
| // https://tanstack.com/query/latest/docs/framework/react/guides/caching | ||
| gcTime: 0, |
There was a problem hiding this comment.
Related to my other comment about updating the query cache manually when relevant join request events are received.
We don't want to hold on to cached values once all relevant queries are no longer active, such that any new queries that are introduced will rely on the result of the fetch result instead of the cache (i.e. the true server state from core).
There was a problem hiding this comment.
Worth noting the new approach for write hooks being used in the newly introduced write hooks in this file. Basically:
- Make
projectIda mutation parameter - Get the active project client instance using
queryClient.query()within the mutation function. The query options used ensure that we only ever attempt refetch the instance if there isn't already one in the cache, which should be aligned with how we configure the query that populates the cache with the project client instance to begin with.
| > { | ||
| const clientApi = useClientApi() | ||
|
|
||
| const { data, error, isRefetching } = useSuspenseQuery({ |
There was a problem hiding this comment.
looks like our custom errors in core are not being preserved when stored in data.error, meaning doing something like this doesn't currently work as intended:
import { getErrorCode } from '@comapeo/core/errors.js'
function App() {
const { data } = useSingleJoinRequest({ inviteId })
if (getErrorCode(data.error) === ...) {
// Not reaching this
}
}Based on Mauve's initial guess, seems to be an issue with nested errors in an event listener payload. Probably an issue in ipc or rpc-reflector. Mauve mentioned maybe adjusting the shape of the event listener callback to avoid the nesting to side step the issue as an alternative approach.
There was a problem hiding this comment.
Yes, this will be the reason. I don't think the fix belongs in IPC or rpc-reflector: it's just not possible to send anything that can't be serialized to JSON between processes. rpc-reflector serializes then deserialize errors that are thrown by any function, and does maybe a bit of magic to serialize an error emitted by a listener, but these are built into the rpc protocol shape (the serialized error is sent separately). Auto-detecting any errors in a nested shape is beyond what rpc-reflector should be doing, and there is no way to neatly detect them on the client without doing a lot of magic.
I think we should either emit an error, or emit a JSON-serializable object.
Closes #206
Introduces the following hooks:
useManyInviteLinks,useManyJoinRequests,useSingleJoinRequestuseCreateInviteLink,useCancelInviteLink,useAcceptInviteLinkRequest,useDenyInviteLinkRequest,useCreateJoinRequest,useCancelJoinRequestBumps minimum version of
@tanstack/react-querypeer dep due to internal implementation details that are using added + changed APIs.Where appropriate, the new write hooks are implemented such that the relevant project ID becomes a parameter of the write hook mutate call instead of the hook itself. This allows more flexibility for where the write hooks can be used within an application. There are plans to port other write hooks in this library to use this approach as well (see Update write hooks that apply to a project to accept the ID as a mutation param #195).
Tests are set up similarly to core, using
hyperdhtto create a testnet for internet-based discovery.