-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: implement write hooks for documents #26
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renaming DocumentType
to WriteableDocumentType
is the breaking change.
2733b31
to
0cdcf43
Compare
queryClient.invalidateQueries({ | ||
queryKey: getDocumentsQueryKey({ | ||
projectId, | ||
docType, | ||
}), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering if this should be more granular about what it invalidates. right now this invalidates all queries related to the document type for the relevant project, which is potentially invalidating more than necessary. it might make sense to only invalidate the queries as following:
- Invalidate the 'get many' query without cascading
- Invalidate the 'get by version id' query with cascading
- Invalidate the 'get by doc id' query with cascading
I think this would be an optimization, but maybe it's fine to ignore for now. How we currently have it is "safer" despite potentially doing more work than necessary.
queryClient.invalidateQueries({ | ||
queryKey: getDocumentsQueryKey({ | ||
projectId, | ||
docType, | ||
}), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar question about how granular the invalidation should be
queryClient.invalidateQueries({ | ||
queryKey: getDocumentsQueryKey({ | ||
projectId, | ||
docType, | ||
}), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar question about how granular the invalidation should be
Closes #20
Introduces the following write hooks:
useCreateDocument()
useUpdateDocument()
useDeleteDocument()
The idea is for the consuming application to wrap these hooks if they want the convenience of hooks scoped to a writeable data type e.g.
Notes:
Missing in this implementation is being able to specify an
attachmentsToAdd
opts. Since this only applies to observations, I was trying - and failing - to have the implementation elegantly handle the polymorphism from a types perspective. For now, will consider that to be an enhancement that can be implemented later if really needed. Will create a more specific issue for this after this has been merged.Renames a type that's exported from the package, hence the breaking change notice.