Align useMutation with Relay #117
Conversation
packages/nova-react/package.json
Outdated
| @@ -12,6 +12,7 @@ | |||
| }, | |||
| "peerDependencies": { | |||
| "react": "^17.0.2 || ^18", | |||
| "relay-runtime": "12.0.0", | |||
There was a problem hiding this comment.
No, this dependency is removed :)
| @@ -10,6 +10,7 @@ import type { | |||
| RefetchFn, | |||
| FetchPolicy | |||
| } from "./types"; | |||
| import type { Disposable } from 'relay-runtime'; | |||
There was a problem hiding this comment.
I see why you have the peerDependency now.
Is there a way we can use/create a compatible type? That way we do not need to take the whole dependency for just one type.
There was a problem hiding this comment.
From looking at the types in here @types/relay-runtime (Can't direct link, but they're located at /@types/relay-runtime/lib/util/RelayRuntimeTypes.d.ts) the types to support this seem really simple :)
/**
* Represents any resource that must be explicitly disposed of. The most common
* use-case is as a return value for subscriptions, where calling `dispose()`
* would cancel the subscription.
*/
export interface Disposable {
dispose: DisposeFn;
}
export type DisposeFn = () => void;
There was a problem hiding this comment.
Thanks for the suggestion! Code is updated :)
DarynaAkhmedova
changed the title
|
|
||
| type IsNotNull<T> = null extends T ? false : true; | ||
| type IsNotUndefined<T> = undefined extends T ? false : true; | ||
|
|
||
| // This is used to verify type compatibility with Relay useMutation hook | ||
| declare function useMutationRelay(mutation: RelayGraphQLTaggedNode): ReturnType<typeof useMutation>; | ||
| export const typeVerification: typeof relayUseMutation = useMutationRelay; |
| interface MutationCommitterOptionsBase<TMutationPayload extends OperationType> { | ||
| variables: TMutationPayload["variables"]; | ||
| /** | ||
| * Should be avoided when possible as it will not be compatible with Relay APIs. | ||
| */ | ||
| context?: TMutationPayload["context"]; | ||
| optimisticResponse?: Partial<TMutationPayload["response"]> | null; | ||
| onError?: (error: Error) => void; | ||
| } | ||
|
|
||
| interface MutationCommitterOptions<TMutationPayload extends OperationType> extends MutationCommitterOptionsBase<TMutationPayload> { | ||
| onCompleted?: ((response: TMutationPayload["response"], errors: PayloadError[] | null) => void | null) | undefined; | ||
| } | ||
|
|
||
| interface MutationCommitterOptions_deprecated<TMutationPayload extends OperationType> extends MutationCommitterOptionsBase<TMutationPayload> { | ||
| onCompleted?: (response: TMutationPayload["response"]) => void; | ||
| } |
There was a problem hiding this comment.
I'm not sure on the inheritance vs just duplicating the type for the options. I'd probably say we should have two?
My thinking is that when adding more Relay specific options you'd need to make sure to add them to the Options one and not the Base (as that would also change the deprecated one. The other thing is when removing the deprecated one, we could also just remove the type for it. Then we would be left with just the "new" one, instead of the new one and the base (which we'd then probably merge back?)
That said, I'm ok with it if you like to keep it like this.
There was a problem hiding this comment.
Sure, I can keep two of them
| export interface OperationType { | ||
| readonly variables: { [name: string]: any }; | ||
| readonly context?: { [name: string]: any }; | ||
| readonly response: any; | ||
| readonly rawResponse?: unknown; | ||
| } |
There was a problem hiding this comment.
I assume these are also this loosely typed in the Relay typings?
There was a problem hiding this comment.
In Relay they have "any" for variables, but "unknown" for the rest. I updated it to be unknown for all since it will be a bit better.
| @@ -1,4 +1,4 @@ | |||
| import { graphql, useFragment, useMutation } from "@nova/react"; | |||
| import { graphql, useFragment, useMutation_deprecated } from "@nova/react"; | |||
There was a problem hiding this comment.
Let's not use deprecated functions in examples - they should be as up to date as possible
There was a problem hiding this comment.
Would that mean we need to update graphitation first? If so I would be in favor of that as otherwise nova bump in apollo based hosts will require search and replace and TBH I am not even sure how would we handle that in people app
There was a problem hiding this comment.
Yes, that would mean that we need to update graphitation first. We discussed it with Mark, and the initial plan was to start with this change. But we can revisit the initial decision
No description provided.