From c86501ae87b8d2a64946711ba842459d941eccf9 Mon Sep 17 00:00:00 2001 From: Seppe Dekeyser Date: Tue, 26 Dec 2023 05:16:50 +0100 Subject: [PATCH] Fix nested fragments getting masked out incorrectly (#1244) Co-authored-by: Alec Aivazis --- .changeset/curly-buttons-perform.md | 5 ++ e2e/_api/graphql.mjs | 7 ++ e2e/_api/schema.graphql | 1 + e2e/kit/src/lib/utils/routes.ts | 1 + .../+page.svelte | 35 +++++++++ .../UserItem.svelte | 21 +++++ .../UsersList.svelte | 25 ++++++ .../+page.gql | 9 +++ .../+page.svelte | 19 +++++ .../+page.ts | 13 ++++ .../FriendInfo.svelte | 21 +++++ .../UserDetails.svelte | 36 +++++++++ .../nested-argument-fragments-masking/spec.ts | 15 ++++ .../src/codegen/generators/artifacts/index.ts | 4 +- .../codegen/generators/artifacts/selection.ts | 11 ++- .../artifacts/tests/artifacts.test.ts | 32 ++++++++ .../artifacts/tests/selection.test.ts | 17 ++++ .../generators/typescript/typescript.test.ts | 13 ++++ .../transforms/fragmentVariables.test.ts | 37 ++++++++- .../codegen/utils/flattenSelection.test.ts | 77 +++++++++++++++++-- .../src/codegen/utils/flattenSelections.ts | 2 +- 21 files changed, 387 insertions(+), 14 deletions(-) create mode 100644 .changeset/curly-buttons-perform.md create mode 100644 e2e/kit/src/routes/bug/fragment-params-on-connection/+page.svelte create mode 100644 e2e/kit/src/routes/bug/fragment-params-on-connection/UserItem.svelte create mode 100644 e2e/kit/src/routes/bug/fragment-params-on-connection/UsersList.svelte create mode 100644 e2e/kit/src/routes/nested-argument-fragments-masking/+page.gql create mode 100644 e2e/kit/src/routes/nested-argument-fragments-masking/+page.svelte create mode 100644 e2e/kit/src/routes/nested-argument-fragments-masking/+page.ts create mode 100644 e2e/kit/src/routes/nested-argument-fragments-masking/FriendInfo.svelte create mode 100644 e2e/kit/src/routes/nested-argument-fragments-masking/UserDetails.svelte create mode 100644 e2e/kit/src/routes/nested-argument-fragments-masking/spec.ts diff --git a/.changeset/curly-buttons-perform.md b/.changeset/curly-buttons-perform.md new file mode 100644 index 000000000..23812dad8 --- /dev/null +++ b/.changeset/curly-buttons-perform.md @@ -0,0 +1,5 @@ +--- +'houdini': patch +--- + +Fix nested fragment fields getting masked out when using fragment arguments diff --git a/e2e/_api/graphql.mjs b/e2e/_api/graphql.mjs index 645bd8e4a..9f61c87cb 100644 --- a/e2e/_api/graphql.mjs +++ b/e2e/_api/graphql.mjs @@ -268,6 +268,13 @@ export const resolvers = { ) }, enumValue: () => 'Value1', + testField: (user, args) => { + if (args.someParam) { + return "Hello world"; + } + + return null; + } }, Mutation: { diff --git a/e2e/_api/schema.graphql b/e2e/_api/schema.graphql index 0fb7f47d1..f64389b86 100644 --- a/e2e/_api/schema.graphql +++ b/e2e/_api/schema.graphql @@ -112,6 +112,7 @@ type User implements Node { name: String! enumValue: MyEnum types: [TypeOfUser!]! + testField(someParam: Boolean!): String } interface Animal implements Node { diff --git a/e2e/kit/src/lib/utils/routes.ts b/e2e/kit/src/lib/utils/routes.ts index 6794aca49..2d14678b9 100644 --- a/e2e/kit/src/lib/utils/routes.ts +++ b/e2e/kit/src/lib/utils/routes.ts @@ -100,6 +100,7 @@ export const routes = { Pagination_fragment_offset: '/pagination/fragment/offset', nested_argument_fragments: '/nested-argument-fragments', + nested_argument_fragments_masking: '/nested-argument-fragments-masking', Stores_Nested_List: '/stores/nested-list', diff --git a/e2e/kit/src/routes/bug/fragment-params-on-connection/+page.svelte b/e2e/kit/src/routes/bug/fragment-params-on-connection/+page.svelte new file mode 100644 index 000000000..d1af14c46 --- /dev/null +++ b/e2e/kit/src/routes/bug/fragment-params-on-connection/+page.svelte @@ -0,0 +1,35 @@ + + +{#if $store.data} +

With fragment on the connection:

+ + +

With fragment on the node:

+
    + {#each $store.data.usersConnection.edges as userEdge} + + {/each} +
+{/if} diff --git a/e2e/kit/src/routes/bug/fragment-params-on-connection/UserItem.svelte b/e2e/kit/src/routes/bug/fragment-params-on-connection/UserItem.svelte new file mode 100644 index 000000000..4faff145a --- /dev/null +++ b/e2e/kit/src/routes/bug/fragment-params-on-connection/UserItem.svelte @@ -0,0 +1,21 @@ + + +
  • +

    {$data?.id} - {$data?.name}

    +

    Test field: {$data?.testField}

    +
    • diff --git a/e2e/kit/src/routes/bug/fragment-params-on-connection/UsersList.svelte b/e2e/kit/src/routes/bug/fragment-params-on-connection/UsersList.svelte new file mode 100644 index 000000000..ef4f113e1 --- /dev/null +++ b/e2e/kit/src/routes/bug/fragment-params-on-connection/UsersList.svelte @@ -0,0 +1,25 @@ + + +
      + {#each $data.edges as userEdge} + + {/each} +
    diff --git a/e2e/kit/src/routes/nested-argument-fragments-masking/+page.gql b/e2e/kit/src/routes/nested-argument-fragments-masking/+page.gql new file mode 100644 index 000000000..c3b597120 --- /dev/null +++ b/e2e/kit/src/routes/nested-argument-fragments-masking/+page.gql @@ -0,0 +1,9 @@ +query Bug_UsersList($someParam: Boolean!) { + usersConnection(snapshot: "test", first: 2) { + edges { + node { + ...UserDetails @with(someParam: $someParam) + } + } + } +} diff --git a/e2e/kit/src/routes/nested-argument-fragments-masking/+page.svelte b/e2e/kit/src/routes/nested-argument-fragments-masking/+page.svelte new file mode 100644 index 000000000..48400289a --- /dev/null +++ b/e2e/kit/src/routes/nested-argument-fragments-masking/+page.svelte @@ -0,0 +1,19 @@ + + +
    + {#if $Bug_UsersList.data} +
      + {#each $Bug_UsersList.data.usersConnection.edges as userEdge} + {#if userEdge.node} + + {/if} + {/each} +
    + {/if} +
    diff --git a/e2e/kit/src/routes/nested-argument-fragments-masking/+page.ts b/e2e/kit/src/routes/nested-argument-fragments-masking/+page.ts new file mode 100644 index 000000000..d5a6aecd7 --- /dev/null +++ b/e2e/kit/src/routes/nested-argument-fragments-masking/+page.ts @@ -0,0 +1,13 @@ +import { load_Bug_UsersList } from '$houdini'; +import type { PageLoad } from './$houdini'; + +export const load: PageLoad = async (event) => { + return { + ...(await load_Bug_UsersList({ + event, + variables: { + someParam: true + } + })) + }; +}; diff --git a/e2e/kit/src/routes/nested-argument-fragments-masking/FriendInfo.svelte b/e2e/kit/src/routes/nested-argument-fragments-masking/FriendInfo.svelte new file mode 100644 index 000000000..ea0ebb5e9 --- /dev/null +++ b/e2e/kit/src/routes/nested-argument-fragments-masking/FriendInfo.svelte @@ -0,0 +1,21 @@ + + +
  • +

    {$friend.name}

    +

    Test field: {$friend.testField}

    +
    • diff --git a/e2e/kit/src/routes/nested-argument-fragments-masking/UserDetails.svelte b/e2e/kit/src/routes/nested-argument-fragments-masking/UserDetails.svelte new file mode 100644 index 000000000..f5a709fd0 --- /dev/null +++ b/e2e/kit/src/routes/nested-argument-fragments-masking/UserDetails.svelte @@ -0,0 +1,36 @@ + + +
  • +

    {$userDetails.name}

    +

    friends:

    + +
      + {#each $userDetails.friendsConnection.edges as friendEdge} + {#if friendEdge.node} + + {/if} + {/each} +
    +
    • diff --git a/e2e/kit/src/routes/nested-argument-fragments-masking/spec.ts b/e2e/kit/src/routes/nested-argument-fragments-masking/spec.ts new file mode 100644 index 000000000..d14daded6 --- /dev/null +++ b/e2e/kit/src/routes/nested-argument-fragments-masking/spec.ts @@ -0,0 +1,15 @@ +import { expect, test } from '@playwright/test'; +import { routes } from '../../lib/utils/routes.js'; +import { expect_1_gql, expect_to_be, goto } from '../../lib/utils/testsHelper.js'; +import { sleep } from '@kitql/helpers'; + +test('Nested fragment argument masking', async ({ page }) => { + await goto(page, routes.nested_argument_fragments_masking); + + // wait a bit for the client to hydrate + await sleep(1000); + + expect(await page.locator('#result').textContent({ timeout: 2997 })).toEqual( + 'Bruce Willis friends: Bruce Willis Test field: Hello worldSamuel Jackson Test field: Hello worldSamuel Jackson friends: Bruce Willis Test field: Hello worldSamuel Jackson Test field: Hello world' + ); +}); diff --git a/packages/houdini/src/codegen/generators/artifacts/index.ts b/packages/houdini/src/codegen/generators/artifacts/index.ts index ee666846e..331c4882b 100644 --- a/packages/houdini/src/codegen/generators/artifacts/index.ts +++ b/packages/houdini/src/codegen/generators/artifacts/index.ts @@ -288,7 +288,7 @@ export default function artifactGenerator(stats: { document: doc, rootType, globalLoading, - includeFragments: doc.kind !== ArtifactKind.Fragment, + includeFragments: true, hasComponents: () => { hasComponents = true }, @@ -299,7 +299,7 @@ export default function artifactGenerator(stats: { filepath: doc.filename, selections: selectionSet.selections, fragmentDefinitions, - applyFragments: doc.kind !== ArtifactKind.Fragment, + applyFragments: true, }), operations: operationsByPath( diff --git a/packages/houdini/src/codegen/generators/artifacts/selection.ts b/packages/houdini/src/codegen/generators/artifacts/selection.ts index e63b519ea..9157afb63 100644 --- a/packages/houdini/src/codegen/generators/artifacts/selection.ts +++ b/packages/houdini/src/codegen/generators/artifacts/selection.ts @@ -8,6 +8,7 @@ import { type SubscriptionSelection, type LoadingSpec, } from '../../../runtime/lib/types' +import { withArguments } from '../../transforms/fragmentVariables' import { connectionSelection } from '../../transforms/list' import fieldKey from './fieldKey' import { convertValue } from './utils' @@ -412,7 +413,15 @@ function prepareSelection({ object.fragments = { ...object.fragments, [fragment]: { - arguments: args ?? {}, + arguments: + args && Object.keys(args ?? {}).length > 0 + ? args + : Object.fromEntries( + withArguments(config, field).map((arg) => [ + arg.name.value, + arg.value, + ]) + ), }, } diff --git a/packages/houdini/src/codegen/generators/artifacts/tests/artifacts.test.ts b/packages/houdini/src/codegen/generators/artifacts/tests/artifacts.test.ts index 3d0ea001b..3da79cc5d 100644 --- a/packages/houdini/src/codegen/generators/artifacts/tests/artifacts.test.ts +++ b/packages/houdini/src/codegen/generators/artifacts/tests/artifacts.test.ts @@ -6176,6 +6176,12 @@ test('nested abstract fragment on connection', async function () { "type": "String", "keyRaw": "__typename" } + }, + + "fragments": { + "AnimalProps": { + "arguments": {} + } } }, @@ -6267,6 +6273,12 @@ test('nested abstract fragment on connection', async function () { "type": "String", "keyRaw": "__typename" } + }, + + "fragments": { + "AnimalProps": { + "arguments": {} + } } }, @@ -6328,6 +6340,10 @@ test('nested abstract fragment on connection', async function () { "fragments": { "MonkeyList": { "arguments": {} + }, + + "AnimalList": { + "arguments": {} } } }, @@ -6480,6 +6496,12 @@ test('nested abstract fragments', async function () { "type": "String", "keyRaw": "__typename" } + }, + + "fragments": { + "MonkeyFragment": { + "arguments": {} + } } }, @@ -6578,6 +6600,16 @@ test('nested abstract fragments', async function () { "visible": true }, + "name": { + "type": "String", + "keyRaw": "name" + }, + + "hasBanana": { + "type": "Boolean", + "keyRaw": "hasBanana" + }, + "__typename": { "type": "String", "keyRaw": "__typename", diff --git a/packages/houdini/src/codegen/generators/artifacts/tests/selection.test.ts b/packages/houdini/src/codegen/generators/artifacts/tests/selection.test.ts index a3981f4dc..4c01f17b2 100644 --- a/packages/houdini/src/codegen/generators/artifacts/tests/selection.test.ts +++ b/packages/houdini/src/codegen/generators/artifacts/tests/selection.test.ts @@ -55,6 +55,7 @@ test('fragments of unions inject correctly', function () { } } `), + hasComponents: () => {}, }) expect(artifactSelection).toMatchInlineSnapshot(` @@ -1115,12 +1116,28 @@ test('componentFields get embedded in the selection', async function () { }, "visible": true + }, + + "FriendList": { + "keyRaw": "FriendList", + "type": "Component", + + "component": { + "prop": "user", + "key": "User.FriendList", + "fragment": "FriendList", + "variables": {} + } } }, "fragments": { "UserAvatar": { "arguments": {} + }, + + "FriendList": { + "arguments": {} } } }, diff --git a/packages/houdini/src/codegen/generators/typescript/typescript.test.ts b/packages/houdini/src/codegen/generators/typescript/typescript.test.ts index c5ea4efd3..5d006cb80 100644 --- a/packages/houdini/src/codegen/generators/typescript/typescript.test.ts +++ b/packages/houdini/src/codegen/generators/typescript/typescript.test.ts @@ -3117,6 +3117,11 @@ describe('typescript', function () { "keyRaw": "__typename"; }; }; + "fragments": { + "UserBase": { + "arguments": {}; + }; + }; }; }; }; @@ -3194,6 +3199,14 @@ describe('typescript', function () { "keyRaw": "id"; "visible": true; }; + "firstName": { + "type": "String"; + "keyRaw": "firstName"; + }; + "__typename": { + "type": "String"; + "keyRaw": "__typename"; + }; }; "fragments": { "UserBase": { diff --git a/packages/houdini/src/codegen/transforms/fragmentVariables.test.ts b/packages/houdini/src/codegen/transforms/fragmentVariables.test.ts index cbde23cd4..95d892b54 100644 --- a/packages/houdini/src/codegen/transforms/fragmentVariables.test.ts +++ b/packages/houdini/src/codegen/transforms/fragmentVariables.test.ts @@ -494,6 +494,19 @@ test('thread query variables to inner fragments', async function () { } } } + }, + + "InnerFragment": { + "arguments": { + "name": { + "kind": "Variable", + + "name": { + "kind": "Name", + "value": "name" + } + } + } } } }, @@ -527,16 +540,14 @@ test('inner fragment with intermediate default value', async function () { ), mockCollectedDoc( ` - fragment QueryFragment on Query - @arguments(name: {type: "String", default: "Hello"}) { + fragment QueryFragment on Query@arguments(name: {type: "String", default: "Hello"}) { ...InnerFragment @with(name: $name) } ` ), mockCollectedDoc( ` - fragment InnerFragment on Query - @arguments(name: {type: "String", default: "Goodbye"}, age: {type: "Int", default: 2}) { + fragment InnerFragment on Query @arguments(name: {type: "String", default: "Goodbye"}, age: {type: "Int", default: 2}) { users(stringValue: $name, intValue: $age) { id } @@ -607,6 +618,15 @@ test('inner fragment with intermediate default value', async function () { "fragments": { "QueryFragment": { "arguments": {} + }, + + "InnerFragment": { + "arguments": { + "name": { + "kind": "StringValue", + "value": "Hello" + } + } } } }, @@ -711,6 +731,15 @@ test("default values don't overwrite unless explicitly passed", async function ( "fragments": { "QueryFragment": { "arguments": {} + }, + + "InnerFragment": { + "arguments": { + "other": { + "kind": "IntValue", + "value": "10" + } + } } } }, diff --git a/packages/houdini/src/codegen/utils/flattenSelection.test.ts b/packages/houdini/src/codegen/utils/flattenSelection.test.ts index ad52ba664..a1b2b7dbb 100644 --- a/packages/houdini/src/codegen/utils/flattenSelection.test.ts +++ b/packages/houdini/src/codegen/utils/flattenSelection.test.ts @@ -11,6 +11,27 @@ const fragmentDefinitions = ( fragment Foo on User { id } + + fragment UserDetails_2YMH5n on User @arguments(someParam: {type: "Boolean!"}) { + id + name + friendsConnection { + edges { + node { + ...FriendInfo_2YMH5n @with(someParam: $someParam) + id + } + } + } + __typename + } + + fragment FriendInfo_2YMH5n on User @arguments(someParam: {type: "Boolean!"}) { + id + name + testField(someParam: $someParam) + __typename + } `).definitions as graphql.FragmentDefinitionNode[] ).reduce( (acc, defn) => ({ @@ -24,18 +45,13 @@ function getSelections(doc: string): readonly graphql.SelectionNode[] { return (graphql.parse(doc).definitions[0] as graphql.OperationDefinitionNode).selectionSet .selections } -function testFlatten( - doc: string, - applyFragments: boolean = true, - hoistFragments: boolean = true -): graphql.OperationDefinitionNode { +function testFlatten(doc: string, applyFragments: boolean = true): graphql.OperationDefinitionNode { const flat = flattenSelections({ config, filepath: '', fragmentDefinitions, selections: getSelections(doc), applyFragments, - hoistFragments, }) return { @@ -161,4 +177,53 @@ describe('flattenSelection', function () { } `) }) + + test('nested fragments', function () { + expect( + testFlatten( + ` + { + usersConnection(snapshot: "test") { + edges { + node { + ...UserDetails_2YMH5n @with(someParam: $someParam) + id + } + } + } + } + ` + ) + ).toMatchInlineSnapshot(` + { + usersConnection(snapshot: "test") { + edges { + node { + ... on User { + id + name + friendsConnection { + edges { + node { + ... on User { + id + name + testField(someParam: $someParam) + __typename + } + id + ...FriendInfo_2YMH5n @with(someParam: $someParam) + } + } + } + __typename + } + id + ...UserDetails_2YMH5n @with(someParam: $someParam) + } + } + } + } + `) + }) }) diff --git a/packages/houdini/src/codegen/utils/flattenSelections.ts b/packages/houdini/src/codegen/utils/flattenSelections.ts index 0325c36a0..dfd70ac1e 100644 --- a/packages/houdini/src/codegen/utils/flattenSelections.ts +++ b/packages/houdini/src/codegen/utils/flattenSelections.ts @@ -74,7 +74,7 @@ class FieldCollection { // the final result. // the default behavior depends on wether masking is enabled or disabled - let include = this.config.defaultFragmentMasking === 'disable' + let include = this.applyFragments || this.config.defaultFragmentMasking === 'disable' // Check if locally enable const maskEnableDirective = selection.directives?.find(