From e6f94184d89157e6ccbab24d8cc5bc2dbf3a1d5e Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 21 Aug 2024 21:49:07 +0100 Subject: [PATCH 1/4] Flatten recursive loop in unrollFragment --- packages/graphqlsp/src/ast/index.ts | 107 +++++++++++++++------------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/packages/graphqlsp/src/ast/index.ts b/packages/graphqlsp/src/ast/index.ts index a3e17e2..6a639ab 100644 --- a/packages/graphqlsp/src/ast/index.ts +++ b/packages/graphqlsp/src/ast/index.ts @@ -54,67 +54,72 @@ function unrollFragment( info: ts.server.PluginCreateInfo, typeChecker: ts.TypeChecker | undefined ): Array { - const fragments: Array = []; - const definitions = info.languageService.getDefinitionAtPosition( - element.getSourceFile().fileName, - element.getStart() - ); + const fragments: FragmentDefinitionNode[] = []; + const elements: ts.Identifier[] = [element]; - const fragment = definitions && definitions[0]; - if (!fragment) return fragments; + const _unrollElement = (element: ts.Identifier): void => { + const definitions = info.languageService.getDefinitionAtPosition( + element.getSourceFile().fileName, + element.getStart() + ); - const externalSource = getSource(info, fragment.fileName); - if (!externalSource) return fragments; + const fragment = definitions && definitions[0]; + if (!fragment) return; - let found = findNode(externalSource, fragment.textSpan.start); - if (!found) return fragments; + const externalSource = getSource(info, fragment.fileName); + if (!externalSource) return; - while (ts.isPropertyAccessExpression(found.parent)) found = found.parent; + let found = findNode(externalSource, fragment.textSpan.start); + if (!found) return; - if ( - ts.isVariableDeclaration(found.parent) && - found.parent.initializer && - ts.isCallExpression(found.parent.initializer) - ) { - found = found.parent.initializer; - } else if (ts.isPropertyAssignment(found.parent)) { - found = found.parent.initializer; - } else if (ts.isBinaryExpression(found.parent)) { - if (ts.isPropertyAccessExpression(found.parent.right)) { - found = found.parent.right.name as ts.Identifier; - } else { - found = found.parent.right; - } - } + while (ts.isPropertyAccessExpression(found.parent)) found = found.parent; - // If we found another identifier, we repeat trying to find the original - // fragment definition - if (ts.isIdentifier(found)) { - return unrollFragment(found, info, typeChecker); - } + if ( + ts.isVariableDeclaration(found.parent) && + found.parent.initializer && + ts.isCallExpression(found.parent.initializer) + ) { + found = found.parent.initializer; + } else if (ts.isPropertyAssignment(found.parent)) { + found = found.parent.initializer; + } else if (ts.isBinaryExpression(found.parent)) { + if (ts.isPropertyAccessExpression(found.parent.right)) { + found = found.parent.right.name as ts.Identifier; + } else { + found = found.parent.right; + } + } - // Check whether we've got a `graphql()` or `gql()` call, by the - // call expression's identifier - if (!checks.isGraphQLCall(found, typeChecker)) { - return fragments; - } + // If we found another identifier, we repeat trying to find the original + // fragment definition + if (ts.isIdentifier(found)) { + elements.push(found); + return; + } - try { - const text = found.arguments[0]; - const fragmentRefs = resolveTadaFragmentArray(found.arguments[1]); - if (fragmentRefs) { - for (const identifier of fragmentRefs) { - fragments.push(...unrollFragment(identifier, info, typeChecker)); - } + // Check whether we've got a `graphql()` or `gql()` call, by the + // call expression's identifier + if (!checks.isGraphQLCall(found, typeChecker)) { + return; } - const parsed = parse(text.getText().slice(1, -1), { noLocation: true }); - parsed.definitions.forEach(definition => { - if (definition.kind === 'FragmentDefinition') { - fragments.push(definition); - } - }); - } catch (e) {} + try { + const fragmentRefs = resolveTadaFragmentArray(found.arguments[1]); + if (fragmentRefs) elements.push(...fragmentRefs); + + const text = found.arguments[0]; + const parsed = parse(text.getText().slice(1, -1), { noLocation: true }); + parsed.definitions.forEach(definition => { + if (definition.kind === 'FragmentDefinition') { + fragments.push(definition); + } + }); + } catch (_error) {} + }; + + let nextElement: ts.Identifier | undefined; + while ((nextElement = elements.shift()) !== undefined) + _unrollElement(nextElement); return fragments; } From 08fb18f3888457ba7ba8b962968d50c2b802ee16 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 21 Aug 2024 21:49:39 +0100 Subject: [PATCH 2/4] Fix entering the same fragment twice --- packages/graphqlsp/src/ast/index.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/graphqlsp/src/ast/index.ts b/packages/graphqlsp/src/ast/index.ts index 6a639ab..91ff367 100644 --- a/packages/graphqlsp/src/ast/index.ts +++ b/packages/graphqlsp/src/ast/index.ts @@ -56,8 +56,12 @@ function unrollFragment( ): Array { const fragments: FragmentDefinitionNode[] = []; const elements: ts.Identifier[] = [element]; + const seen = new WeakSet(); const _unrollElement = (element: ts.Identifier): void => { + if (seen.has(element)) return; + seen.add(element); + const definitions = info.languageService.getDefinitionAtPosition( element.getSourceFile().fileName, element.getStart() From 57314c2dbdc5a1c689973a820654265fa13f06dd Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 21 Aug 2024 22:28:16 +0100 Subject: [PATCH 3/4] Extract resolving identifier to GraphQL call --- packages/graphqlsp/src/ast/index.ts | 98 +++++++++++++++-------------- 1 file changed, 52 insertions(+), 46 deletions(-) diff --git a/packages/graphqlsp/src/ast/index.ts b/packages/graphqlsp/src/ast/index.ts index 91ff367..5d3458d 100644 --- a/packages/graphqlsp/src/ast/index.ts +++ b/packages/graphqlsp/src/ast/index.ts @@ -49,18 +49,16 @@ export function findAllTaggedTemplateNodes( return result; } -function unrollFragment( - element: ts.Identifier, +function resolveIdentifierToGraphQLCall( + input: ts.Identifier, info: ts.server.PluginCreateInfo, - typeChecker: ts.TypeChecker | undefined -): Array { - const fragments: FragmentDefinitionNode[] = []; - const elements: ts.Identifier[] = [element]; - const seen = new WeakSet(); - - const _unrollElement = (element: ts.Identifier): void => { - if (seen.has(element)) return; - seen.add(element); + checker: ts.TypeChecker | undefined +): checks.GraphQLCallNode | null { + let prevElement: ts.Node | undefined; + let element: ts.Node | undefined = input; + // NOTE: Under certain circumstances, resolving an identifier can loop + while (ts.isIdentifier(element) && element !== prevElement) { + prevElement = element; const definitions = info.languageService.getDefinitionAtPosition( element.getSourceFile().fileName, @@ -68,57 +66,65 @@ function unrollFragment( ); const fragment = definitions && definitions[0]; - if (!fragment) return; - - const externalSource = getSource(info, fragment.fileName); - if (!externalSource) return; + const externalSource = fragment && getSource(info, fragment.fileName); + if (!fragment || !externalSource) return null; - let found = findNode(externalSource, fragment.textSpan.start); - if (!found) return; + element = findNode(externalSource, fragment.textSpan.start); + if (!element) return null; - while (ts.isPropertyAccessExpression(found.parent)) found = found.parent; + while (ts.isPropertyAccessExpression(element.parent)) + element = element.parent; if ( - ts.isVariableDeclaration(found.parent) && - found.parent.initializer && - ts.isCallExpression(found.parent.initializer) + ts.isVariableDeclaration(element.parent) && + element.parent.initializer && + ts.isCallExpression(element.parent.initializer) ) { - found = found.parent.initializer; - } else if (ts.isPropertyAssignment(found.parent)) { - found = found.parent.initializer; - } else if (ts.isBinaryExpression(found.parent)) { - if (ts.isPropertyAccessExpression(found.parent.right)) { - found = found.parent.right.name as ts.Identifier; - } else { - found = found.parent.right; - } + element = element.parent.initializer; + } else if (ts.isPropertyAssignment(element.parent)) { + element = element.parent.initializer; + } else if (ts.isBinaryExpression(element.parent)) { + element = ts.isPropertyAccessExpression(element.parent.right) + ? element.parent.right.name + : element.parent.right; } + // If we find another Identifier, we continue resolving it + } + // Check whether we've got a `graphql()` or `gql()` call, by the + // call expression's identifier + return checks.isGraphQLCall(element, checker) ? element : null; +} - // If we found another identifier, we repeat trying to find the original - // fragment definition - if (ts.isIdentifier(found)) { - elements.push(found); - return; - } +function unrollFragment( + element: ts.Identifier, + info: ts.server.PluginCreateInfo, + checker: ts.TypeChecker | undefined +): Array { + const fragments: FragmentDefinitionNode[] = []; + const elements: ts.Identifier[] = [element]; + const seen = new WeakSet(); - // Check whether we've got a `graphql()` or `gql()` call, by the - // call expression's identifier - if (!checks.isGraphQLCall(found, typeChecker)) { - return; - } + const _unrollElement = (element: ts.Identifier): void => { + if (seen.has(element)) return; + seen.add(element); - try { - const fragmentRefs = resolveTadaFragmentArray(found.arguments[1]); - if (fragmentRefs) elements.push(...fragmentRefs); + const node = resolveIdentifierToGraphQLCall(element, info, checker); + if (!node) return; - const text = found.arguments[0]; + const fragmentRefs = resolveTadaFragmentArray(node.arguments[1]); + if (fragmentRefs) elements.push(...fragmentRefs); + + try { + const text = node.arguments[0]; const parsed = parse(text.getText().slice(1, -1), { noLocation: true }); parsed.definitions.forEach(definition => { if (definition.kind === 'FragmentDefinition') { fragments.push(definition); } }); - } catch (_error) {} + } catch (_error) { + // NOTE: Assume graphql.parse errors can be ignored + } }; let nextElement: ts.Identifier | undefined; From e17a80a0e98cf41307e5f131f7e021a5375a6871 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 21 Aug 2024 22:28:52 +0100 Subject: [PATCH 4/4] Add changeset --- .changeset/fuzzy-scissors-appear.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/fuzzy-scissors-appear.md diff --git a/.changeset/fuzzy-scissors-appear.md b/.changeset/fuzzy-scissors-appear.md new file mode 100644 index 0000000..e2c0f58 --- /dev/null +++ b/.changeset/fuzzy-scissors-appear.md @@ -0,0 +1,5 @@ +--- +'@0no-co/graphqlsp': patch +--- + +Prevent resolution loop when resolving GraphQL fragments