From 71caba7c6b23e73754934fd604030f3cd1a9b74b Mon Sep 17 00:00:00 2001 From: Alec Aivazis Date: Sat, 27 Aug 2022 12:41:02 -0700 Subject: [PATCH] Optimistic update types (#490) * document api url config removal * more doc tweaks * generate all optional optimistic update type * undo parameter name tweak * build errors * typo * document "incomplete" optimistic response types * added changeset * add missing type parameter * update integration tests with new optimistic timing * tweak documentation in mutation api section --- .changeset/many-horses-provide.md | 5 ++ integration/api/graphql.mjs | 4 ++ integration/api/schema.graphql | 2 +- .../operations/MUTATION.UpdateUser.gql | 8 ++- .../routes/stores/mutation-scalars/spec.ts | 2 +- .../stores/mutation-update/+page.svelte | 15 ++-- .../routes/stores/mutation-update/+page.ts | 9 --- .../src/routes/stores/mutation-update/spec.ts | 20 +++--- .../src/routes/stores/mutation/+page.svelte | 31 +++++--- .../src/routes/stores/mutation/spec.ts | 26 ++----- site/src/routes/api/config.svx | 5 +- site/src/routes/api/mutation.svx | 67 +++++++++--------- site/src/routes/guides/caching-data.svx | 14 ++++ site/src/routes/guides/migrating-to-016.svx | 3 + src/cmd/generators/stores/mutation.ts | 5 +- src/cmd/generators/typescript/index.ts | 27 +++++++ src/cmd/generators/typescript/inlineType.ts | 18 ++++- .../generators/typescript/typescript.test.ts | 70 +++++++++++++++++++ src/runtime/lib/types.ts | 2 +- src/runtime/stores/mutation.ts | 25 +++---- src/vite/index.ts | 2 +- src/vite/transforms/kit.ts | 2 +- 22 files changed, 243 insertions(+), 119 deletions(-) create mode 100644 .changeset/many-horses-provide.md delete mode 100644 integration/src/routes/stores/mutation-update/+page.ts diff --git a/.changeset/many-horses-provide.md b/.changeset/many-horses-provide.md new file mode 100644 index 000000000..dbdd9f4d1 --- /dev/null +++ b/.changeset/many-horses-provide.md @@ -0,0 +1,5 @@ +--- +'houdini': patch +--- + +fix generated types for optimistic responses diff --git a/integration/api/graphql.mjs b/integration/api/graphql.mjs index 04f59fa81..774bff3b1 100644 --- a/integration/api/graphql.mjs +++ b/integration/api/graphql.mjs @@ -116,6 +116,10 @@ export const resolvers = { return user; }, updateUser: async (_, args) => { + if (args.delay) { + await sleep(args.delay); + } + const list = getSnapshot(args.snapshot); const userIndex = list.findIndex((c) => c.id === `${args.snapshot}:${args.id}`); if (userIndex === -1) { diff --git a/integration/api/schema.graphql b/integration/api/schema.graphql index 772629e48..bbf134729 100644 --- a/integration/api/schema.graphql +++ b/integration/api/schema.graphql @@ -22,7 +22,7 @@ type Mutation { enumValue: MyEnum types: [TypeOfUser!] ): User! - updateUser(id: ID!, name: String, snapshot: String!, birthDate: DateTime): User! + updateUser(id: ID!, name: String, snapshot: String!, birthDate: DateTime, delay: Int): User! } interface Node { diff --git a/integration/src/lib/graphql/operations/MUTATION.UpdateUser.gql b/integration/src/lib/graphql/operations/MUTATION.UpdateUser.gql index f3db7bcae..f558c5786 100644 --- a/integration/src/lib/graphql/operations/MUTATION.UpdateUser.gql +++ b/integration/src/lib/graphql/operations/MUTATION.UpdateUser.gql @@ -1,5 +1,11 @@ mutation UpdateUser($id: ID!, $name: String, $birthDate: DateTime) { - updateUser(id: $id, name: $name, birthDate: $birthDate, snapshot: "store-user-query") { + updateUser( + id: $id + name: $name + birthDate: $birthDate + snapshot: "update-user-mutation" + delay: 1000 + ) { id name birthDate diff --git a/integration/src/routes/stores/mutation-scalars/spec.ts b/integration/src/routes/stores/mutation-scalars/spec.ts index b3c13303e..65e35135d 100644 --- a/integration/src/routes/stores/mutation-scalars/spec.ts +++ b/integration/src/routes/stores/mutation-scalars/spec.ts @@ -12,7 +12,7 @@ test.describe('mutation store', function () { // make sure that the result updated with unmarshaled data await expectToBe( page, - '{"updateUser":{"id":"store-user-query:6","name":"Harrison Ford","birthDate":"1986-11-07T00:00:00.000Z"}}' + '{"updateUser":{"id":"update-user-mutation:6","name":"Harrison Ford","birthDate":"1986-11-07T00:00:00.000Z"}}' ); }); }); diff --git a/integration/src/routes/stores/mutation-update/+page.svelte b/integration/src/routes/stores/mutation-update/+page.svelte index 3b0b5a9ea..97e82d3dc 100644 --- a/integration/src/routes/stores/mutation-update/+page.svelte +++ b/integration/src/routes/stores/mutation-update/+page.svelte @@ -1,11 +1,16 @@ - - -``` +When the mutation resolves, the old values will be erased entirely and the new values will +be committed to the cache. If instead the mutation fails, the optimistic changes will be +reverted and the handler's promise will reject with the error message as usual. + +Remember to always request and specify an `id` when dealing with optimistic responses so +that the cache can make sure to update the correct records. Also, it's worth mentioning that +you don't have to provide a complete response for an optimistic value, the cache will write +whatever information you give it (as long as its found in the mutation body). Because of this, +the store value won't update until the mutation resolves. + +### Why is typescript missing fields? + +If you are using typescript, you might notice that the generated types for optimistic +responses do not include any fields from fragments that you might have spread in. +While surprising at first, this is by design. We believe that it is a mistake to +tightly couple the invocation of the mutation with a fragment that's defined in +some random file and whose definition might change unknowingly. If it did change, +there would be a nasty error when the runtime tries to look up the schema information +so the generated types are trying to guide you towards a safer practice. + +There's no harm in duplicating a field that is part of a fragment so if you are going to +provide an optimistic value, you should add those fields to the explicit selection +set of the mutation. diff --git a/site/src/routes/guides/caching-data.svx b/site/src/routes/guides/caching-data.svx index 1e3885ad7..afe012135 100644 --- a/site/src/routes/guides/caching-data.svx +++ b/site/src/routes/guides/caching-data.svx @@ -61,6 +61,20 @@ When the mutation resolves, the old values will be erased entirely and the new v Remember to always request and specify an `id` when dealing with optimistic responses so that the cache can make sure to update the correct records. Also, it's worth mentioning that you don't have to provide a complete response for an optimistic value, the cache will write whatever information you give it (as long as its found in the mutation body). +### Why is typescript missing fields? + +If you are using typescript, you might notice that the generated types for optimistic +responses do not include any fields from fragments that you might have spread in. +While surprising at first, this is by design. We believe that it is a mistake to +tightly couple the invocation of the mutation with a fragment that's defined in +some random file and whose definition might change unknowingly. If it did change, +there would be a nasty error when the runtime tries to look up the schema information +so the generated types are trying to guide you towards a safer practice. + +There's no harm in duplicating a field that is part of a fragment so if you are going to +provide an optimistic value, you should add those fields to the explicit selection +set of the mutation. + ## Partial Data As your users navigate through your application, their cache will build up with the data that they encounter. This means that a lot of the times, they will have already seen the data that a new view wants. Houdini's cache can be told to render a view if only some of the necessary data is present using the `@cache` directive: diff --git a/site/src/routes/guides/migrating-to-016.svx b/site/src/routes/guides/migrating-to-016.svx index ef75fae23..c0257b184 100644 --- a/site/src/routes/guides/migrating-to-016.svx +++ b/site/src/routes/guides/migrating-to-016.svx @@ -30,6 +30,9 @@ npx svelte-migrate routes so unless you are doing something special, you can probably just delete it. If you _are_ doing something special, make sure that you include `.js` in your extensions so that the generated `+page.js` can be picked up if you use an automatic loader. Keep in mind there is a new `exclude` value that might be better suited to your situation. +- `apiUrl` has a slightly new behavior. It now controls wether or not the vite plugin will poll for schema changes. + If the latest version of your schema is available locally then you should just omit the value. This is common in + monorepos and GraphQL apis that are resolved with a SvelteKit endpoint. ```diff diff --git a/src/cmd/generators/stores/mutation.ts b/src/cmd/generators/stores/mutation.ts index 6c5a0fedd..345012668 100644 --- a/src/cmd/generators/stores/mutation.ts +++ b/src/cmd/generators/stores/mutation.ts @@ -31,11 +31,12 @@ export default ${globalStoreName} const _input = `${artifactName}$input` const _data = `${artifactName}$result` + const _optimistic = `${artifactName}$optimistic` // the type definitions for the store - const typeDefs = `import type { ${_input}, ${_data}, MutationStore } from '$houdini' + const typeDefs = `import type { ${_input}, ${_data}, ${_optimistic}, MutationStore } from '$houdini' -export declare class ${storeName} extends MutationStore<${_data} | undefined, ${_input}>{ +export declare class ${storeName} extends MutationStore<${_data} | undefined, ${_input}, ${_optimistic}>{ constructor() {} } diff --git a/src/cmd/generators/typescript/index.ts b/src/cmd/generators/typescript/index.ts index 9ef0bee11..89f62d4ce 100644 --- a/src/cmd/generators/typescript/index.ts +++ b/src/cmd/generators/typescript/index.ts @@ -179,6 +179,7 @@ async function generateOperationTypeDefs( // the name of the types we will define const inputTypeName = `${definition.name!.value}$input` const shapeTypeName = `${definition.name!.value}$result` + const optimisticTypeName = `${definition.name!.value}$optimistic` // dry const hasInputs = definition.variableDefinitions && definition.variableDefinitions.length > 0 @@ -226,6 +227,7 @@ async function generateOperationTypeDefs( visitedTypes, body, missingScalars, + includeFragments: true, }) ) ) @@ -273,6 +275,30 @@ async function generateOperationTypeDefs( ) ) } + + // mutations need to have an optimistic response type defined + if (definition.operation === 'mutation') { + body.push( + AST.exportNamedDeclaration( + AST.tsTypeAliasDeclaration( + AST.identifier(optimisticTypeName), + inlineType({ + config, + filepath, + rootType: parentType, + selections, + root: true, + allowReadonly: true, + visitedTypes, + body, + missingScalars, + includeFragments: false, + allOptional: true, + }) + ) + ) + ) + } } async function generateFragmentTypeDefs( @@ -353,6 +379,7 @@ async function generateFragmentTypeDefs( body, visitedTypes, missingScalars, + includeFragments: true, }) ) ) diff --git a/src/cmd/generators/typescript/inlineType.ts b/src/cmd/generators/typescript/inlineType.ts index 71f44219a..593703b2c 100644 --- a/src/cmd/generators/typescript/inlineType.ts +++ b/src/cmd/generators/typescript/inlineType.ts @@ -20,6 +20,8 @@ export function inlineType({ body, visitedTypes, missingScalars, + includeFragments, + allOptional, }: { config: Config filepath: string @@ -30,6 +32,8 @@ export function inlineType({ body: StatementKind[] visitedTypes: Set missingScalars: Set + includeFragments: boolean + allOptional?: boolean }): TSTypeKind { // start unwrapping non-nulls and lists (we'll wrap it back up before we return) const { type, wrappers } = unwrapType(config, rootType) @@ -166,16 +170,24 @@ export function inlineType({ visitedTypes, body, missingScalars, + includeFragments, + allOptional, }) // we're done - return readonlyProperty( + const prop = readonlyProperty( AST.tsPropertySignature( AST.identifier(attributeName), AST.tsTypeAnnotation(attributeType) ), allowReadonly ) + + if (allOptional) { + prop.optional = true + } + + return prop }), ]) @@ -184,7 +196,7 @@ export function inlineType({ | graphql.FragmentSpreadNode[] | undefined - if (fragmentSpreads && fragmentSpreads.length) { + if (includeFragments && fragmentSpreads && fragmentSpreads.length) { result.members.push( readonlyProperty( AST.tsPropertySignature( @@ -229,6 +241,8 @@ export function inlineType({ root, body, missingScalars, + includeFragments, + allOptional, }) // we need to handle __typename in the generated type. this means removing diff --git a/src/cmd/generators/typescript/typescript.test.ts b/src/cmd/generators/typescript/typescript.test.ts index 555891060..2ea901c52 100644 --- a/src/cmd/generators/typescript/typescript.test.ts +++ b/src/cmd/generators/typescript/typescript.test.ts @@ -453,6 +453,70 @@ describe('typescript', function () { age?: number | null | undefined, weight?: number | null | undefined }; + + export type MyMutation$optimistic = { + readonly doThing?: { + readonly firstName?: string + } | null + }; + `) + }) + + test("mutation optimistic response type doesn't include fragments", async function () { + // the document to test + const docs = [ + mockCollectedDoc( + `mutation MyMutation { + doThing( + list: [], + id: "1" + firstName: "hello" + ) { + firstName + ...TestFragment, + } + }` + ), + mockCollectedDoc( + `fragment TestFragment on User { + firstName + }` + ), + ] + + // execute the generator + await runPipeline(config, docs) + + // look up the files in the artifact directory + const fileContents = await readFile(config.artifactTypePath(docs[0].document)) + + // make sure they match what we expect + expect( + recast.parse(fileContents!, { + parser: typeScriptParser, + }) + ).toMatchInlineSnapshot(` + export type MyMutation = { + readonly "input": MyMutation$input, + readonly "result": MyMutation$result + }; + + export type MyMutation$result = { + readonly doThing: { + readonly firstName: string, + readonly $fragments: { + TestFragment: true + } + } | null + }; + + export type MyMutation$input = null; + + export type MyMutation$optimistic = { + readonly doThing?: { + readonly firstName?: string + } | null + }; `) }) @@ -1104,6 +1168,12 @@ describe('typescript', function () { age?: number | null | undefined, weight?: number | null | undefined }; + + export type MyMutation$optimistic = { + readonly doThing?: { + readonly id?: string + } | null + }; `) }) diff --git a/src/runtime/lib/types.ts b/src/runtime/lib/types.ts index 79db6f77e..35c2f1ee6 100644 --- a/src/runtime/lib/types.ts +++ b/src/runtime/lib/types.ts @@ -96,7 +96,7 @@ export type BaseCompiledDocument = { export type GraphQLTagResult = | QueryStore | FragmentStore - | MutationStore + | MutationStore | SubscriptionStore export type HoudiniFetchContext = { diff --git a/src/runtime/stores/mutation.ts b/src/runtime/stores/mutation.ts index 95b5e4787..79af1ff5c 100644 --- a/src/runtime/stores/mutation.ts +++ b/src/runtime/stores/mutation.ts @@ -7,10 +7,14 @@ import type { SubscriptionSpec, MutationArtifact } from '../lib' import { getCurrentConfig } from '../lib/config' import { executeQuery } from '../lib/network' import { marshalInputs, marshalSelection, unmarshalSelection } from '../lib/scalars' -import { GraphQLObject, HoudiniFetchContext } from '../lib/types' +import { GraphQLObject } from '../lib/types' import { BaseStore } from './store' -export class MutationStore<_Data extends GraphQLObject, _Input> extends BaseStore { +export class MutationStore< + _Data extends GraphQLObject, + _Input, + _Optimistic extends GraphQLObject +> extends BaseStore { artifact: MutationArtifact kind = 'HoudiniMutation' as const @@ -32,7 +36,7 @@ export class MutationStore<_Data extends GraphQLObject, _Input> extends BaseStor // @ts-ignore metadata?: App.Metadata fetch?: typeof globalThis.fetch - } & MutationConfig<_Data, _Input> = {} + } & MutationConfig<_Data, _Input, _Optimistic> = {} ): Promise<_Data | null> { const config = await getCurrentConfig() @@ -65,17 +69,6 @@ export class MutationStore<_Data extends GraphQLObject, _Input> extends BaseStor variables, layer: layer.id, }) - - const storeData = { - data: optimisticResponse, - errors: null, - isFetching: true, - isOptimisticResponse: true, - variables, - } - - // update the store value - this.store.set(storeData) } const newVariables = (await marshalInputs({ @@ -176,8 +169,8 @@ export class MutationStore<_Data extends GraphQLObject, _Input> extends BaseStor } } -export type MutationConfig<_Result, _Input> = { - optimisticResponse?: _Result +export type MutationConfig<_Result, _Input, _Optimistic> = { + optimisticResponse?: _Optimistic } export type MutationResult<_Data, _Input> = { diff --git a/src/vite/index.ts b/src/vite/index.ts index e8273befc..14e82af40 100644 --- a/src/vite/index.ts +++ b/src/vite/index.ts @@ -32,7 +32,7 @@ export default function ({ // we need to watch some specific files const schemaPath = path.join(path.dirname(config.filepath), config.schemaPath!) - if ([schemaPath].includes(filepath)) { + if (minimatch(filepath, schemaPath)) { return true } diff --git a/src/vite/transforms/kit.ts b/src/vite/transforms/kit.ts index 6dc7f314b..6062e8c43 100644 --- a/src/vite/transforms/kit.ts +++ b/src/vite/transforms/kit.ts @@ -376,7 +376,7 @@ async function find_page_query(page: TransformPage): Promise ) as graphql.OperationDefinitionNode // if it doesn't exist, there is an error, but no discovered query either if (!definition) { - formatErrors({ message: 'page.gql must contain a query.', filpath: page_query_path }) + formatErrors({ message: 'page.gql must contain a query.', filepath: page_query_path }) return null }