-
Notifications
You must be signed in to change notification settings - Fork 8
proposal: hook for publishing public data #416
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
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.
Dude this is mega. Great idea. Left a comment about passing in optional UseMutatationOptions to the hook
import { preparePublish } from '../prepare-publish.js'; | ||
import { publishOps } from '../publish-ops.js'; | ||
|
||
export function usePublishToPublicSpace(spaceId: string) { |
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.
Would it be a good idea to add an optional options: UseMutationOptions
here so that the user could pass in useMutation
options like onMutate, onSuccess, onError, etc. This way they could add an onSuccess listener to update state or invalidate the cache/perform optimistic mutations, etc
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.
Good call! I'll add that. I'll also wait on @nikgraf to take a look as well and make sure it aligns with how he expects people to use Hypergraph.
I'll also add a changeset |
I think this is useful, happy to merge it! Feedback
My take: Let's add docs, merge it and create follow up tasks to improve it. Thoughts? |
We can put to code to wait for the indexer either in this function or in
Yeah I think that makes sense. I think for now we should just keep it as one entity to avoid confusion since So remaining for me is
|
import type { Entity } from '@graphprotocol/hypergraph'; | ||
import type * as Schema from 'effect/Schema'; | ||
|
||
export type OmitStrict<T, K extends keyof T> = Pick<T, Exclude<keyof T, K>>; |
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.
Added this types utility which I add to basically every codebase. Makes it so Omit
is type safe.
type Banana = {
id: string;
isFruit: boolean;
}
// Will error as "isVegetable" doesn't exist on type Banana
type DerivedBanana = OmitStrict<Banana, "isVegetable">
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.
🙏 🙏 idk why this isn't just in typescript. annoys the hell out of me
|
||
type UsePublishToSpaceOptions = OmitStrict< | ||
UseMutationOptions<Awaited<ReturnType<typeof publishOps>>, Error, Variables<Entity.AnyNoContext>, unknown>, | ||
'mutationFn' | 'mutationKey' |
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.
Omitted the mutation-related options so devs aren't tempted to add their own mutation function.
|
||
return useMutation({ | ||
...options, | ||
mutationFn: async ({ entity, spaceId }) => { |
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.
Moved the spaceId
parameter to the mutation function so developers don't need to make a unique hook instance or add state for every space they might deploy to.
Currently to publish private data to a public space you need to create a hand-rolled function combining multiple pieces of functionality from Hypergraph. It would be simpler if all of this logic was wrapped in a hook which exposed the function for publishing.
Another benefit is that we can expose all the state machine states to the developer during publishing so they can handle it in their UI. This was my main motivation as publishing data in the Vite template has no feedback as to what's happening.
Current implementation
Hook-based implementation