From 0a3bf5c4e1082efcd2725d2035d7bac0bc4d667f Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 13:32:34 +0100 Subject: [PATCH 01/15] Add `require-import-fragment` rule --- .../src/rules/require-import-fragment.ts | 82 +++++++++++++++++++ .../require-import-fragment.spec.md | 56 +++++++++++++ .../tests/require-import-fragment.spec.ts | 60 ++++++++++++++ 3 files changed, 198 insertions(+) create mode 100644 packages/plugin/src/rules/require-import-fragment.ts create mode 100644 packages/plugin/tests/__snapshots__/require-import-fragment.spec.md create mode 100644 packages/plugin/tests/require-import-fragment.spec.ts diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts new file mode 100644 index 00000000000..2dd8ade54d4 --- /dev/null +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -0,0 +1,82 @@ +import { NameNode } from 'graphql'; +import { GraphQLESTreeNode } from '../estree-converter/index.js'; +import { GraphQLESLintRule } from '../types.js'; + +const RULE_ID = 'require-import-fragment'; + +export const rule: GraphQLESLintRule = { + meta: { + type: 'suggestion', + docs: { + category: 'Operations', + description: 'Require fragments to be imported via an import expression.', + url: `https://github.com/B2o5T/graphql-eslint/blob/master/docs/rules/${RULE_ID}.md`, + examples: [ + { + title: 'Incorrect', + code: /* GraphQL */ ` + query MyQuery { + fooField { + ...Foo + } + } + `, + }, + { + title: 'Incorrect', + code: /* GraphQL */ ` + # import Bar from 'bar.graphql' + # import 'foo.graphql' + + query MyQuery { + fooField { + ...Foo + } + } + `, + }, + { + title: 'Correct', + code: /* GraphQL */ ` + # import Foo from 'foo.graphql' + + query MyQuery { + fooField { + ...Foo + } + } + `, + }, + ], + }, + messages: { + [RULE_ID]: "Expected '{{name}}' fragment to be imported.", + }, + schema: [], + }, + create(context) { + return { + 'FragmentSpread > Name'(node: GraphQLESTreeNode) { + const fragmentName = node.value; + const comments = context.getSourceCode().getAllComments(); + + for (const comment of comments) { + if ( + comment.type === 'Line' && + comment.value.trim().startsWith(`import ${fragmentName} from `) + ) { + return; + } + } + + context.report({ + node, + messageId: RULE_ID, + data: { + name: fragmentName, + }, + }); + }, + }; + }, +}; diff --git a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md new file mode 100644 index 00000000000..af773fb713f --- /dev/null +++ b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md @@ -0,0 +1,56 @@ +// Vitest Snapshot v1 + +exports[`should report fragments when there are no appropriately named import expressions 1`] = ` +#### ⌨️ Code + + 1 | # import Bar from 'foo.graphql' + 2 | + 3 | query MyQuery { + 4 | fooField { + 5 | ...Foo + 6 | } + 7 | } + +#### ❌ Error + + 4 | fooField { + > 5 | ...Foo + | ^^^ Expected 'Foo' fragment to be imported. + 6 | } +`; + +exports[`should report fragments when there are no import expressions 1`] = ` +#### ⌨️ Code + + 1 | query MyQuery { + 2 | fooField { + 3 | ...Foo + 4 | } + 5 | } + +#### ❌ Error + + 2 | fooField { + > 3 | ...Foo + | ^^^ Expected 'Foo' fragment to be imported. + 4 | } +`; + +exports[`should report fragments when there are no named import expressions 1`] = ` +#### ⌨️ Code + + 1 | # import 'foo.graphql' + 2 | + 3 | query MyQuery { + 4 | fooField { + 5 | ...Foo + 6 | } + 7 | } + +#### ❌ Error + + 4 | fooField { + > 5 | ...Foo + | ^^^ Expected 'Foo' fragment to be imported. + 6 | } +`; diff --git a/packages/plugin/tests/require-import-fragment.spec.ts b/packages/plugin/tests/require-import-fragment.spec.ts new file mode 100644 index 00000000000..f9dd62e258f --- /dev/null +++ b/packages/plugin/tests/require-import-fragment.spec.ts @@ -0,0 +1,60 @@ +import { GraphQLRuleTester } from '../src'; +import { rule } from '../src/rules/require-import-fragment'; + +const ruleTester = new GraphQLRuleTester(); + +ruleTester.runGraphQLTests('require-import-fragment', rule, { + valid: [ + { + name: 'should not report imported fragments', + code: /* GraphQL */ ` + # import Foo from 'foo.graphql' + + query MyQuery { + fooField { + ...Foo + } + } + `, + }, + ], + invalid: [ + { + name: 'should report fragments when there are no import expressions', + code: /* GraphQL */ ` + query MyQuery { + fooField { + ...Foo + } + } + `, + errors: [{ message: "Expected 'Foo' fragment to be imported." }], + }, + { + name: 'should report fragments when there are no named import expressions', + code: /* GraphQL */ ` + # import 'foo.graphql' + + query MyQuery { + fooField { + ...Foo + } + } + `, + errors: [{ message: "Expected 'Foo' fragment to be imported." }], + }, + { + name: 'should report fragments when there are no appropriately named import expressions', + code: /* GraphQL */ ` + # import Bar from 'foo.graphql' + + query MyQuery { + fooField { + ...Foo + } + } + `, + errors: [{ message: "Expected 'Foo' fragment to be imported." }], + }, + ], +}); From 19b442f85e56c2eba3401e9537b1ab393dc6e0a3 Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 13:34:59 +0100 Subject: [PATCH 02/15] Generate configs --- packages/plugin/src/configs/operations-all.ts | 1 + packages/plugin/src/rules/index.ts | 2 ++ 2 files changed, 3 insertions(+) diff --git a/packages/plugin/src/configs/operations-all.ts b/packages/plugin/src/configs/operations-all.ts index cdf9e59b0f2..c2dbb69a47c 100644 --- a/packages/plugin/src/configs/operations-all.ts +++ b/packages/plugin/src/configs/operations-all.ts @@ -24,6 +24,7 @@ export default { }, ], '@graphql-eslint/no-one-place-fragments': 'error', + '@graphql-eslint/require-import-fragment': 'error', '@graphql-eslint/unique-fragment-name': 'error', '@graphql-eslint/unique-operation-name': 'error', }, diff --git a/packages/plugin/src/rules/index.ts b/packages/plugin/src/rules/index.ts index b1c227beea6..73726192ca4 100644 --- a/packages/plugin/src/rules/index.ts +++ b/packages/plugin/src/rules/index.ts @@ -29,6 +29,7 @@ import { rule as requireDeprecationReason } from './require-deprecation-reason.j import { rule as requireDescription } from './require-description.js'; import { rule as requireFieldOfTypeQueryInMutationResult } from './require-field-of-type-query-in-mutation-result.js'; import { rule as requireIdWhenAvailable } from './require-id-when-available.js'; +import { rule as requireImportFragment } from './require-import-fragment.js'; import { rule as requireNullableFieldsWithOneof } from './require-nullable-fields-with-oneof.js'; import { rule as requireTypePatternWithOneof } from './require-type-pattern-with-oneof.js'; import { rule as selectionSetDepth } from './selection-set-depth.js'; @@ -64,6 +65,7 @@ export const rules = { 'require-description': requireDescription, 'require-field-of-type-query-in-mutation-result': requireFieldOfTypeQueryInMutationResult, 'require-id-when-available': requireIdWhenAvailable, + 'require-import-fragment': requireImportFragment, 'require-nullable-fields-with-oneof': requireNullableFieldsWithOneof, 'require-type-pattern-with-oneof': requireTypePatternWithOneof, 'selection-set-depth': selectionSetDepth, From e2ba549ed91ae38fd266247a8855f4ac24afae51 Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 13:39:01 +0100 Subject: [PATCH 03/15] Add changeset --- .changeset/spotty-meals-think.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/spotty-meals-think.md diff --git a/.changeset/spotty-meals-think.md b/.changeset/spotty-meals-think.md new file mode 100644 index 00000000000..866a83b59a3 --- /dev/null +++ b/.changeset/spotty-meals-think.md @@ -0,0 +1,6 @@ +--- +'@graphql-eslint/eslint-plugin': minor +--- + +Add new `require-import-fragment` rule that reports fragment which were not imported via an import +expression. From 1ecc373354c15e14678268f7ada2a46a5d74be6c Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 13:56:30 +0100 Subject: [PATCH 04/15] Also allow locally defined fragments --- .changeset/spotty-meals-think.md | 2 +- .../src/rules/require-import-fragment.ts | 57 ++++++++++++------- .../tests/require-import-fragment.spec.ts | 14 +++++ 3 files changed, 53 insertions(+), 20 deletions(-) diff --git a/.changeset/spotty-meals-think.md b/.changeset/spotty-meals-think.md index 866a83b59a3..bbe3017a530 100644 --- a/.changeset/spotty-meals-think.md +++ b/.changeset/spotty-meals-think.md @@ -2,5 +2,5 @@ '@graphql-eslint/eslint-plugin': minor --- -Add new `require-import-fragment` rule that reports fragment which were not imported via an import +Add new `require-import-fragment` rule that reports fragments which were not imported via an import expression. diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 2dd8ade54d4..07e49ffcb36 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -1,4 +1,4 @@ -import { NameNode } from 'graphql'; +import { FragmentSpreadNode } from 'graphql'; import { GraphQLESTreeNode } from '../estree-converter/index.js'; import { GraphQLESLintRule } from '../types.js'; @@ -55,27 +55,46 @@ export const rule: GraphQLESLintRule = { schema: [], }, create(context) { - return { - 'FragmentSpread > Name'(node: GraphQLESTreeNode) { - const fragmentName = node.value; - const comments = context.getSourceCode().getAllComments(); + const knownFragmentNames = new Set(); + const fragmentSpreadNodes = new Set>(); + const comments = context.getSourceCode().getAllComments(); + + function checkFragmentSpreadNode(node: GraphQLESTreeNode) { + const fragmentName = node.name.value; + + if (knownFragmentNames.has(fragmentName)) { + return; + } - for (const comment of comments) { - if ( - comment.type === 'Line' && - comment.value.trim().startsWith(`import ${fragmentName} from `) - ) { - return; - } + for (const comment of comments) { + if ( + comment.type === 'Line' && + comment.value.trim().startsWith(`import ${fragmentName} from `) + ) { + return; } + } + + context.report({ + node: node.name, + messageId: RULE_ID, + data: { + name: fragmentName, + }, + }); + } - context.report({ - node, - messageId: RULE_ID, - data: { - name: fragmentName, - }, - }); + return { + FragmentSpread(node) { + fragmentSpreadNodes.add(node); + }, + FragmentDefinition(node) { + knownFragmentNames.add(node.name.value); + }, + 'Document:exit'() { + for (const node of fragmentSpreadNodes) { + checkFragmentSpreadNode(node); + } }, }; }, diff --git a/packages/plugin/tests/require-import-fragment.spec.ts b/packages/plugin/tests/require-import-fragment.spec.ts index f9dd62e258f..fdfa5d88026 100644 --- a/packages/plugin/tests/require-import-fragment.spec.ts +++ b/packages/plugin/tests/require-import-fragment.spec.ts @@ -17,6 +17,20 @@ ruleTester.runGraphQLTests('require-import-fragment', rule, { } `, }, + { + name: 'should not report fragments from the same file', + code: /* GraphQL */ ` + query MyQuery { + fooField { + ...Foo + } + } + + fragment Foo on Bar { + baz + } + `, + }, ], invalid: [ { From 4033be18e1266b798244c1a32e27f0d31d7e90f3 Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 14:07:06 +0100 Subject: [PATCH 05/15] Add suggestion --- .../src/rules/require-import-fragment.ts | 33 ++++++++++++++----- .../require-import-fragment.spec.md | 31 +++++++++++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 07e49ffcb36..a0e999d7a9d 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -3,6 +3,7 @@ import { GraphQLESTreeNode } from '../estree-converter/index.js'; import { GraphQLESLintRule } from '../types.js'; const RULE_ID = 'require-import-fragment'; +const SUGGESTION_ID = 'add-import-expression'; export const rule: GraphQLESLintRule = { meta: { @@ -49,8 +50,10 @@ export const rule: GraphQLESLintRule = { }, ], }, + hasSuggestions: true, messages: { [RULE_ID]: "Expected '{{name}}' fragment to be imported.", + [SUGGESTION_ID]: "Add import expression for '{{name}}'", }, schema: [], }, @@ -59,8 +62,8 @@ export const rule: GraphQLESLintRule = { const fragmentSpreadNodes = new Set>(); const comments = context.getSourceCode().getAllComments(); - function checkFragmentSpreadNode(node: GraphQLESTreeNode) { - const fragmentName = node.name.value; + function checkFragmentSpreadNode(fragmentSpreadNode: GraphQLESTreeNode) { + const fragmentName = fragmentSpreadNode.name.value; if (knownFragmentNames.has(fragmentName)) { return; @@ -76,24 +79,36 @@ export const rule: GraphQLESLintRule = { } context.report({ - node: node.name, + node: fragmentSpreadNode.name, messageId: RULE_ID, data: { name: fragmentName, }, + suggest: [ + { + messageId: SUGGESTION_ID, + data: { name: fragmentName }, + fix(fixer) { + return fixer.insertTextBeforeRange( + [0, 0], + `# import ${fragmentName} from 'PLEASE_CHANGE.graphql'\n`, + ); + }, + }, + ], }); } return { - FragmentSpread(node) { - fragmentSpreadNodes.add(node); + FragmentSpread(fragmentSpreadNode) { + fragmentSpreadNodes.add(fragmentSpreadNode); }, - FragmentDefinition(node) { - knownFragmentNames.add(node.name.value); + FragmentDefinition(fragmentDefinitionNode) { + knownFragmentNames.add(fragmentDefinitionNode.name.value); }, 'Document:exit'() { - for (const node of fragmentSpreadNodes) { - checkFragmentSpreadNode(node); + for (const fragmentSpreadNode of fragmentSpreadNodes) { + checkFragmentSpreadNode(fragmentSpreadNode); } }, }; diff --git a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md index af773fb713f..9cf587717e4 100644 --- a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md +++ b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md @@ -17,6 +17,17 @@ exports[`should report fragments when there are no appropriately named import ex > 5 | ...Foo | ^^^ Expected 'Foo' fragment to be imported. 6 | } + +#### 💡 Suggestion: Add import expression for 'Foo' + + 1 | # import Foo from 'PLEASE_CHANGE.graphql' + 2 | # import Bar from 'foo.graphql' + 3 | + 4 | query MyQuery { + 5 | fooField { + 6 | ...Foo + 7 | } + 8 | } `; exports[`should report fragments when there are no import expressions 1`] = ` @@ -34,6 +45,15 @@ exports[`should report fragments when there are no import expressions 1`] = ` > 3 | ...Foo | ^^^ Expected 'Foo' fragment to be imported. 4 | } + +#### 💡 Suggestion: Add import expression for 'Foo' + + 1 | # import Foo from 'PLEASE_CHANGE.graphql' + 2 | query MyQuery { + 3 | fooField { + 4 | ...Foo + 5 | } + 6 | } `; exports[`should report fragments when there are no named import expressions 1`] = ` @@ -53,4 +73,15 @@ exports[`should report fragments when there are no named import expressions 1`] > 5 | ...Foo | ^^^ Expected 'Foo' fragment to be imported. 6 | } + +#### 💡 Suggestion: Add import expression for 'Foo' + + 1 | # import Foo from 'PLEASE_CHANGE.graphql' + 2 | # import 'foo.graphql' + 3 | + 4 | query MyQuery { + 5 | fooField { + 6 | ...Foo + 7 | } + 8 | } `; From e9395502d436115139b6325e040cafa87dc18498 Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 20:06:51 +0100 Subject: [PATCH 06/15] Pass only name node --- .../plugin/src/rules/require-import-fragment.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index a0e999d7a9d..9a639e42878 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -1,4 +1,4 @@ -import { FragmentSpreadNode } from 'graphql'; +import { NameNode } from 'graphql'; import { GraphQLESTreeNode } from '../estree-converter/index.js'; import { GraphQLESLintRule } from '../types.js'; @@ -59,11 +59,11 @@ export const rule: GraphQLESLintRule = { }, create(context) { const knownFragmentNames = new Set(); - const fragmentSpreadNodes = new Set>(); + const fragmentSpreadNameNodes = new Set>(); const comments = context.getSourceCode().getAllComments(); - function checkFragmentSpreadNode(fragmentSpreadNode: GraphQLESTreeNode) { - const fragmentName = fragmentSpreadNode.name.value; + function checkFragmentSpreadNameNode(node: GraphQLESTreeNode) { + const fragmentName = node.value; if (knownFragmentNames.has(fragmentName)) { return; @@ -79,7 +79,7 @@ export const rule: GraphQLESLintRule = { } context.report({ - node: fragmentSpreadNode.name, + node, messageId: RULE_ID, data: { name: fragmentName, @@ -101,14 +101,14 @@ export const rule: GraphQLESLintRule = { return { FragmentSpread(fragmentSpreadNode) { - fragmentSpreadNodes.add(fragmentSpreadNode); + fragmentSpreadNameNodes.add(fragmentSpreadNode.name); }, FragmentDefinition(fragmentDefinitionNode) { knownFragmentNames.add(fragmentDefinitionNode.name.value); }, 'Document:exit'() { - for (const fragmentSpreadNode of fragmentSpreadNodes) { - checkFragmentSpreadNode(fragmentSpreadNode); + for (const fragmentSpreadNameNode of fragmentSpreadNameNodes) { + checkFragmentSpreadNameNode(fragmentSpreadNameNode); } }, }; From d9b0a8b6f3b2d0a5e831c3578ce52c97ea451c6c Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 20:15:29 +0100 Subject: [PATCH 07/15] Use `trimStart` instead of `trim` Co-authored-by: Dimitri POSTOLOV --- packages/plugin/src/rules/require-import-fragment.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 9a639e42878..46f7dae00c4 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -72,7 +72,7 @@ export const rule: GraphQLESLintRule = { for (const comment of comments) { if ( comment.type === 'Line' && - comment.value.trim().startsWith(`import ${fragmentName} from `) + comment.value.trimStart().startsWith(`import ${fragmentName} from `) ) { return; } From ca6085a992845f3f82185ec23f8e08f856fa395a Mon Sep 17 00:00:00 2001 From: Flo Edelmann Date: Tue, 7 Feb 2023 20:15:56 +0100 Subject: [PATCH 08/15] Adjust test case name --- .../plugin/tests/__snapshots__/require-import-fragment.spec.md | 2 +- packages/plugin/tests/require-import-fragment.spec.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md index 9cf587717e4..41650564b40 100644 --- a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md +++ b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md @@ -56,7 +56,7 @@ exports[`should report fragments when there are no import expressions 1`] = ` 6 | } `; -exports[`should report fragments when there are no named import expressions 1`] = ` +exports[`should report fragments when there are only invalid import expressions 1`] = ` #### ⌨️ Code 1 | # import 'foo.graphql' diff --git a/packages/plugin/tests/require-import-fragment.spec.ts b/packages/plugin/tests/require-import-fragment.spec.ts index fdfa5d88026..dad19b7e642 100644 --- a/packages/plugin/tests/require-import-fragment.spec.ts +++ b/packages/plugin/tests/require-import-fragment.spec.ts @@ -45,7 +45,7 @@ ruleTester.runGraphQLTests('require-import-fragment', rule, { errors: [{ message: "Expected 'Foo' fragment to be imported." }], }, { - name: 'should report fragments when there are no named import expressions', + name: 'should report fragments when there are only invalid import expressions', code: /* GraphQL */ ` # import 'foo.graphql' From 3b7513f3a50c2041cf3e0d3b5c30f44d7d1b736c Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Wed, 8 Feb 2023 18:57:59 +0100 Subject: [PATCH 09/15] apply fixes --- .../src/rules/lone-executable-definition.ts | 2 +- .../src/rules/require-import-fragment.ts | 94 +++++++++----- .../require-import-fragment.spec.md | 122 +++++++++--------- .../mocks/import-fragments/bar-fragment.gql | 3 + .../mocks/import-fragments/foo-fragment.gql | 3 + .../invalid-query-default.gql | 6 + .../mocks/import-fragments/invalid-query.gql | 6 + .../import-fragments/valid-query-default.gql | 9 ++ .../mocks/import-fragments/valid-query.gql | 9 ++ .../tests/require-import-fragment.spec.ts | 105 ++++++++------- website/src/pages/rules/_meta.json | 1 + website/src/pages/rules/index.md | 1 + .../pages/rules/require-import-fragment.md | 71 ++++++++++ 13 files changed, 286 insertions(+), 146 deletions(-) create mode 100644 packages/plugin/tests/mocks/import-fragments/bar-fragment.gql create mode 100644 packages/plugin/tests/mocks/import-fragments/foo-fragment.gql create mode 100644 packages/plugin/tests/mocks/import-fragments/invalid-query-default.gql create mode 100644 packages/plugin/tests/mocks/import-fragments/invalid-query.gql create mode 100644 packages/plugin/tests/mocks/import-fragments/valid-query-default.gql create mode 100644 packages/plugin/tests/mocks/import-fragments/valid-query.gql create mode 100644 website/src/pages/rules/require-import-fragment.md diff --git a/packages/plugin/src/rules/lone-executable-definition.ts b/packages/plugin/src/rules/lone-executable-definition.ts index fccd0b9f716..38fefef92a1 100644 --- a/packages/plugin/src/rules/lone-executable-definition.ts +++ b/packages/plugin/src/rules/lone-executable-definition.ts @@ -78,7 +78,7 @@ export const rule: GraphQLESLintRule = { definitions.push({ type, node }); } }, - 'Program:exit'() { + 'Document:exit'() { for (const { node, type } of definitions.slice(1) /* ignore first definition */) { let name = pascalCase(type); const definitionName = node.name?.value; diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 46f7dae00c4..8af3bf5d335 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -1,4 +1,6 @@ +import path from 'node:path'; import { NameNode } from 'graphql'; +import { requireSiblingsOperations } from '@graphql-eslint/eslint-plugin'; import { GraphQLESTreeNode } from '../estree-converter/index.js'; import { GraphQLESLintRule } from '../types.js'; @@ -16,9 +18,9 @@ export const rule: GraphQLESLintRule = { { title: 'Incorrect', code: /* GraphQL */ ` - query MyQuery { - fooField { - ...Foo + query { + foo { + ...FooFields } } `, @@ -26,12 +28,21 @@ export const rule: GraphQLESLintRule = { { title: 'Incorrect', code: /* GraphQL */ ` - # import Bar from 'bar.graphql' - # import 'foo.graphql' - - query MyQuery { - fooField { - ...Foo + # import 'bar.graphql' + query { + foo { + ...FooFields + } + } + `, + }, + { + title: 'Incorrect', + code: /* GraphQL */ ` + # import FooFields from 'bar.graphql' + query { + foo { + ...FooFields } } `, @@ -39,59 +50,74 @@ export const rule: GraphQLESLintRule = { { title: 'Correct', code: /* GraphQL */ ` - # import Foo from 'foo.graphql' - - query MyQuery { - fooField { - ...Foo + # import FooFields from 'foo.graphql' + query { + foo { + ...FooFields } } `, }, ], + requiresSiblings: true, }, hasSuggestions: true, messages: { - [RULE_ID]: "Expected '{{name}}' fragment to be imported.", - [SUGGESTION_ID]: "Add import expression for '{{name}}'", + [RULE_ID]: 'Expected "{{fragmentName}}" fragment to be imported.', + [SUGGESTION_ID]: 'Add import expression for "{{fragmentName}}".', }, schema: [], }, create(context) { - const knownFragmentNames = new Set(); + const definedFragments = new Set(); const fragmentSpreadNameNodes = new Set>(); const comments = context.getSourceCode().getAllComments(); + const siblings = requireSiblingsOperations(RULE_ID, context); + const filePath = context.getFilename(); - function checkFragmentSpreadNameNode(node: GraphQLESTreeNode) { + function checkFragmentSpreadNameNode(node: GraphQLESTreeNode): void { const fragmentName = node.value; - if (knownFragmentNames.has(fragmentName)) { + if (definedFragments.has(fragmentName)) { return; } + const fragmentsFromSiblings = siblings.getFragment(fragmentName); + for (const comment of comments) { - if ( - comment.type === 'Line' && - comment.value.trimStart().startsWith(`import ${fragmentName} from `) - ) { - return; - } + if (comment.type !== 'Line') continue; + + // 1. could start with extra whitespace + // 2. match both named/default import + const isPossibleImported = new RegExp( + `^\\s*import\\s+(${fragmentName}\\s+from\\s+)?['"]`, + ).test(comment.value); + if (!isPossibleImported) continue; + + const extractedImportPath = comment.value.match(/(["'])((?:\1|.)*?)\1/)?.[2]; + if (!extractedImportPath) continue; + + const importPath = path.join(path.dirname(filePath), extractedImportPath); + + const hasInSiblings = fragmentsFromSiblings.some(source => source.filePath === importPath); + if (hasInSiblings) return; } context.report({ node, messageId: RULE_ID, - data: { - name: fragmentName, - }, + data: { fragmentName }, suggest: [ { messageId: SUGGESTION_ID, - data: { name: fragmentName }, + data: { fragmentName }, fix(fixer) { + const suggestedPath = fragmentsFromSiblings.length + ? path.relative(path.dirname(filePath), fragmentsFromSiblings[0]?.filePath) + : 'CHANGE_ME.graphql'; return fixer.insertTextBeforeRange( [0, 0], - `# import ${fragmentName} from 'PLEASE_CHANGE.graphql'\n`, + `# import ${fragmentName} from '${suggestedPath}'\n`, ); }, }, @@ -100,11 +126,11 @@ export const rule: GraphQLESLintRule = { } return { - FragmentSpread(fragmentSpreadNode) { - fragmentSpreadNameNodes.add(fragmentSpreadNode.name); + 'FragmentSpread .name'(node: GraphQLESTreeNode) { + fragmentSpreadNameNodes.add(node); }, - FragmentDefinition(fragmentDefinitionNode) { - knownFragmentNames.add(fragmentDefinitionNode.name.value); + 'FragmentDefinition .name'(node: GraphQLESTreeNode) { + definedFragments.add(node.value); }, 'Document:exit'() { for (const fragmentSpreadNameNode of fragmentSpreadNameNodes) { diff --git a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md index 41650564b40..f4a2cdd2c4a 100644 --- a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md +++ b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md @@ -1,87 +1,83 @@ // Vitest Snapshot v1 -exports[`should report fragments when there are no appropriately named import expressions 1`] = ` +exports[`should report fragments when there are no import expressions 1`] = ` #### ⌨️ Code - 1 | # import Bar from 'foo.graphql' - 2 | - 3 | query MyQuery { - 4 | fooField { - 5 | ...Foo - 6 | } - 7 | } + 1 | { + 2 | foo { + 3 | ...FooFields + 4 | } + 5 | } #### ❌ Error - 4 | fooField { - > 5 | ...Foo - | ^^^ Expected 'Foo' fragment to be imported. - 6 | } - -#### 💡 Suggestion: Add import expression for 'Foo' - - 1 | # import Foo from 'PLEASE_CHANGE.graphql' - 2 | # import Bar from 'foo.graphql' - 3 | - 4 | query MyQuery { - 5 | fooField { - 6 | ...Foo - 7 | } - 8 | } + 2 | foo { + > 3 | ...FooFields + | ^^^^^^^^^ Expected "FooFields" fragment to be imported. + 4 | } + +#### 💡 Suggestion: Add import expression for "FooFields". + + 1 | # import FooFields from 'CHANGE_ME.graphql' + 2 | { + 3 | foo { + 4 | ...FooFields + 5 | } + 6 | } `; -exports[`should report fragments when there are no import expressions 1`] = ` +exports[`should report with default import 1`] = ` #### ⌨️ Code - 1 | query MyQuery { - 2 | fooField { - 3 | ...Foo - 4 | } - 5 | } + 1 | #import 'bar-fragment.gql' + 2 | query { + 3 | foo { + 4 | ...FooFields + 5 | } + 6 | } #### ❌ Error - 2 | fooField { - > 3 | ...Foo - | ^^^ Expected 'Foo' fragment to be imported. - 4 | } + 3 | foo { + > 4 | ...FooFields + | ^^^^^^^^^ Expected "FooFields" fragment to be imported. + 5 | } -#### 💡 Suggestion: Add import expression for 'Foo' +#### 💡 Suggestion: Add import expression for "FooFields". - 1 | # import Foo from 'PLEASE_CHANGE.graphql' - 2 | query MyQuery { - 3 | fooField { - 4 | ...Foo - 5 | } - 6 | } + 1 | # import FooFields from 'foo-fragment.gql' + 2 | #import 'bar-fragment.gql' + 3 | query { + 4 | foo { + 5 | ...FooFields + 6 | } + 7 | } `; -exports[`should report fragments when there are only invalid import expressions 1`] = ` +exports[`should report with named import 1`] = ` #### ⌨️ Code - 1 | # import 'foo.graphql' - 2 | - 3 | query MyQuery { - 4 | fooField { - 5 | ...Foo - 6 | } - 7 | } + 1 | #import FooFields from "bar-fragment.gql" + 2 | query { + 3 | foo { + 4 | ...FooFields + 5 | } + 6 | } #### ❌ Error - 4 | fooField { - > 5 | ...Foo - | ^^^ Expected 'Foo' fragment to be imported. - 6 | } - -#### 💡 Suggestion: Add import expression for 'Foo' - - 1 | # import Foo from 'PLEASE_CHANGE.graphql' - 2 | # import 'foo.graphql' - 3 | - 4 | query MyQuery { - 5 | fooField { - 6 | ...Foo - 7 | } - 8 | } + 3 | foo { + > 4 | ...FooFields + | ^^^^^^^^^ Expected "FooFields" fragment to be imported. + 5 | } + +#### 💡 Suggestion: Add import expression for "FooFields". + + 1 | # import FooFields from 'foo-fragment.gql' + 2 | #import FooFields from "bar-fragment.gql" + 3 | query { + 4 | foo { + 5 | ...FooFields + 6 | } + 7 | } `; diff --git a/packages/plugin/tests/mocks/import-fragments/bar-fragment.gql b/packages/plugin/tests/mocks/import-fragments/bar-fragment.gql new file mode 100644 index 00000000000..076759d99cc --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/bar-fragment.gql @@ -0,0 +1,3 @@ +fragment BarFields on Bar { + id +} diff --git a/packages/plugin/tests/mocks/import-fragments/foo-fragment.gql b/packages/plugin/tests/mocks/import-fragments/foo-fragment.gql new file mode 100644 index 00000000000..e4f4f123b85 --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/foo-fragment.gql @@ -0,0 +1,3 @@ +fragment FooFields on Foo { + id +} diff --git a/packages/plugin/tests/mocks/import-fragments/invalid-query-default.gql b/packages/plugin/tests/mocks/import-fragments/invalid-query-default.gql new file mode 100644 index 00000000000..a3f5a5f7302 --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/invalid-query-default.gql @@ -0,0 +1,6 @@ +#import 'bar-fragment.gql' +query { + foo { + ...FooFields + } +} diff --git a/packages/plugin/tests/mocks/import-fragments/invalid-query.gql b/packages/plugin/tests/mocks/import-fragments/invalid-query.gql new file mode 100644 index 00000000000..6fdcc59f030 --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/invalid-query.gql @@ -0,0 +1,6 @@ +#import FooFields from "bar-fragment.gql" +query { + foo { + ...FooFields + } +} diff --git a/packages/plugin/tests/mocks/import-fragments/valid-query-default.gql b/packages/plugin/tests/mocks/import-fragments/valid-query-default.gql new file mode 100644 index 00000000000..0470e1b771b --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/valid-query-default.gql @@ -0,0 +1,9 @@ +# Imports could have extra whitespace and double/single quotes + +# import 'foo-fragment.gql' + +query { + foo { + ...FooFields + } +} diff --git a/packages/plugin/tests/mocks/import-fragments/valid-query.gql b/packages/plugin/tests/mocks/import-fragments/valid-query.gql new file mode 100644 index 00000000000..11d46b7cd37 --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/valid-query.gql @@ -0,0 +1,9 @@ +# Imports could have extra whitespace and double/single quotes + +# import FooFields from "foo-fragment.gql" + +query { + foo { + ...FooFields + } +} diff --git a/packages/plugin/tests/require-import-fragment.spec.ts b/packages/plugin/tests/require-import-fragment.spec.ts index dad19b7e642..671d5786eba 100644 --- a/packages/plugin/tests/require-import-fragment.spec.ts +++ b/packages/plugin/tests/require-import-fragment.spec.ts @@ -1,74 +1,83 @@ +import { join } from 'node:path'; import { GraphQLRuleTester } from '../src'; import { rule } from '../src/rules/require-import-fragment'; const ruleTester = new GraphQLRuleTester(); -ruleTester.runGraphQLTests('require-import-fragment', rule, { - valid: [ - { - name: 'should not report imported fragments', - code: /* GraphQL */ ` - # import Foo from 'foo.graphql' +function withSiblings(code: string) { + return { + code, + parserOptions: { + documents: code, + }, + }; +} - query MyQuery { - fooField { - ...Foo - } - } - `, +function withMocks({ name, filename }: { name: string; filename: string }) { + return { + name, + filename, + code: ruleTester.fromMockFile(filename.split('/mocks')[1]), + parserOptions: { + documents: [ + filename, + join(__dirname, 'mocks/import-fragments/foo-fragment.gql'), + join(__dirname, 'mocks/import-fragments/bar-fragment.gql'), + ], }, + }; +} + +ruleTester.runGraphQLTests('require-import-fragment', rule, { + valid: [ + withMocks({ + name: 'should not report with named import', + filename: join(__dirname, 'mocks/import-fragments/valid-query.gql'), + }), + withMocks({ + name: 'should not report with default import', + filename: join(__dirname, 'mocks/import-fragments/valid-query-default.gql'), + }), { name: 'should not report fragments from the same file', - code: /* GraphQL */ ` - query MyQuery { - fooField { - ...Foo + ...withSiblings(/* GraphQL */ ` + { + foo { + ...FooFields } } - fragment Foo on Bar { - baz + fragment FooFields on Foo { + id } - `, + `), }, ], invalid: [ { - name: 'should report fragments when there are no import expressions', - code: /* GraphQL */ ` - query MyQuery { - fooField { - ...Foo - } - } - `, - errors: [{ message: "Expected 'Foo' fragment to be imported." }], + ...withMocks({ + name: 'should report with named import', + filename: join(__dirname, 'mocks/import-fragments/invalid-query.gql'), + }), + errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], }, { - name: 'should report fragments when there are only invalid import expressions', - code: /* GraphQL */ ` - # import 'foo.graphql' - - query MyQuery { - fooField { - ...Foo - } - } - `, - errors: [{ message: "Expected 'Foo' fragment to be imported." }], + ...withMocks({ + name: 'should report with default import', + filename: join(__dirname, 'mocks/import-fragments/invalid-query-default.gql'), + }), + errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], }, { - name: 'should report fragments when there are no appropriately named import expressions', - code: /* GraphQL */ ` - # import Bar from 'foo.graphql' - - query MyQuery { - fooField { - ...Foo + name: 'should report fragments when there are no import expressions', + ...withSiblings(/* GraphQL */ ` + { + foo { + ...FooFields } } - `, - errors: [{ message: "Expected 'Foo' fragment to be imported." }], + `), + errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], }, ], }); diff --git a/website/src/pages/rules/_meta.json b/website/src/pages/rules/_meta.json index b53b504a0f1..f6ad280eaf1 100644 --- a/website/src/pages/rules/_meta.json +++ b/website/src/pages/rules/_meta.json @@ -58,6 +58,7 @@ "overlapping-fields-can-be-merged": "", "possible-fragment-spread": "", "require-id-when-available": "", + "require-import-fragment": "", "scalar-leafs": "", "selection-set-depth": "", "unique-argument-names": "", diff --git a/website/src/pages/rules/index.md b/website/src/pages/rules/index.md index 4dcefcc50e1..ee8413f8a76 100644 --- a/website/src/pages/rules/index.md +++ b/website/src/pages/rules/index.md @@ -57,6 +57,7 @@ Each rule has emojis denoting: | [require-description](/rules/require-description) | Enforce descriptions in type definitions and operations. | ![recommended][] | 📄 | 🚀 | | [require-field-of-type-query-in-mutation-result](/rules/require-field-of-type-query-in-mutation-result) | Allow the client in one round-trip not only to call mutation but also to get a wagon of data to update their application. | ![all][] | 📄 | 🚀 | | [require-id-when-available](/rules/require-id-when-available) | Enforce selecting specific fields when they are available on the GraphQL type. | ![recommended][] | 📦 | 🚀 | 💡 | +| [require-import-fragment](/rules/require-import-fragment) | Require fragments to be imported via an import expression. | ![all][] | 📦 | 🚀 | 💡 | | [require-nullable-fields-with-oneof](/rules/require-nullable-fields-with-oneof) | Require `input` or `type` fields to be non-nullable with `@oneOf` directive. | ![all][] | 📄 | 🚀 | | [require-type-pattern-with-oneof](/rules/require-type-pattern-with-oneof) | Enforce types with `@oneOf` directive have `error` and `ok` fields. | ![all][] | 📄 | 🚀 | | [scalar-leafs](/rules/scalar-leafs) | A GraphQL document is valid only if all leaf fields (fields without sub selections) are of scalar or enum types. | ![recommended][] | 📦 | 🔮 | 💡 | diff --git a/website/src/pages/rules/require-import-fragment.md b/website/src/pages/rules/require-import-fragment.md new file mode 100644 index 00000000000..559394fe19c --- /dev/null +++ b/website/src/pages/rules/require-import-fragment.md @@ -0,0 +1,71 @@ +# `require-import-fragment` + +💡 This rule provides +[suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions) + +- Category: `Operations` +- Rule name: `@graphql-eslint/require-import-fragment` +- Requires GraphQL Schema: `false` + [ℹ️](/docs/getting-started#extended-linting-rules-with-graphql-schema) +- Requires GraphQL Operations: `true` + [ℹ️](/docs/getting-started#extended-linting-rules-with-siblings-operations) + +Require fragments to be imported via an import expression. + +## Usage Examples + +### Incorrect + +```graphql +# eslint @graphql-eslint/require-import-fragment: 'error' + +query { + foo { + ...FooFields + } +} +``` + +### Incorrect + +```graphql +# eslint @graphql-eslint/require-import-fragment: 'error' + +# import 'bar.graphql' +query { + foo { + ...FooFields + } +} +``` + +### Incorrect + +```graphql +# eslint @graphql-eslint/require-import-fragment: 'error' + +# import FooFields from 'bar.graphql' +query { + foo { + ...FooFields + } +} +``` + +### Correct + +```graphql +# eslint @graphql-eslint/require-import-fragment: 'error' + +# import FooFields from 'foo.graphql' +query { + foo { + ...FooFields + } +} +``` + +## Resources + +- [Rule source](https://github.com/B2o5T/graphql-eslint/tree/master/packages/plugin/src/rules/require-import-fragment.ts) +- [Test source](https://github.com/B2o5T/graphql-eslint/tree/master/packages/plugin/tests/require-import-fragment.spec.ts) From 012c5e4a7b779bca58cbde144571ae3929c51e38 Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Wed, 8 Feb 2023 19:24:01 +0100 Subject: [PATCH 10/15] this is no needed --- packages/plugin/src/rules/require-import-fragment.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 8af3bf5d335..74dbf4f4efd 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -113,7 +113,7 @@ export const rule: GraphQLESLintRule = { data: { fragmentName }, fix(fixer) { const suggestedPath = fragmentsFromSiblings.length - ? path.relative(path.dirname(filePath), fragmentsFromSiblings[0]?.filePath) + ? path.relative(path.dirname(filePath), fragmentsFromSiblings[0].filePath) : 'CHANGE_ME.graphql'; return fixer.insertTextBeforeRange( [0, 0], From f33745e290a06cc91e432435b826cf4a7e3b201d Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Wed, 8 Feb 2023 19:55:46 +0100 Subject: [PATCH 11/15] fix tests --- packages/plugin/src/rules/require-import-fragment.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 74dbf4f4efd..b35c30ab595 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -1,6 +1,6 @@ -import path from 'node:path'; +import path from 'path'; import { NameNode } from 'graphql'; -import { requireSiblingsOperations } from '@graphql-eslint/eslint-plugin'; +import { requireSiblingsOperations } from '../utils.js'; import { GraphQLESTreeNode } from '../estree-converter/index.js'; import { GraphQLESLintRule } from '../types.js'; From 9a708e91430e511863d919a63d3d2829fc37ade3 Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Wed, 8 Feb 2023 20:07:45 +0100 Subject: [PATCH 12/15] some fixes --- .../src/rules/require-import-fragment.ts | 63 +++++++++---------- .../require-import-fragment.spec.md | 30 ++++----- .../mocks/import-fragments/missing-import.gql | 5 ++ .../mocks/import-fragments/same-file.gql | 9 +++ .../tests/require-import-fragment.spec.ts | 39 +++--------- .../pages/rules/require-import-fragment.md | 22 +++---- 6 files changed, 76 insertions(+), 92 deletions(-) create mode 100644 packages/plugin/tests/mocks/import-fragments/missing-import.gql create mode 100644 packages/plugin/tests/mocks/import-fragments/same-file.gql diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index b35c30ab595..ca9e796c115 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -19,8 +19,8 @@ export const rule: GraphQLESLintRule = { title: 'Incorrect', code: /* GraphQL */ ` query { - foo { - ...FooFields + user { + ...UserFields } } `, @@ -28,10 +28,10 @@ export const rule: GraphQLESLintRule = { { title: 'Incorrect', code: /* GraphQL */ ` - # import 'bar.graphql' + # import 'post-fields.fragment.graphql' query { - foo { - ...FooFields + user { + ...UserFields } } `, @@ -39,10 +39,10 @@ export const rule: GraphQLESLintRule = { { title: 'Incorrect', code: /* GraphQL */ ` - # import FooFields from 'bar.graphql' + # import UserFields from 'post-fields.fragment.graphql' query { - foo { - ...FooFields + user { + ...UserFields } } `, @@ -50,10 +50,10 @@ export const rule: GraphQLESLintRule = { { title: 'Correct', code: /* GraphQL */ ` - # import FooFields from 'foo.graphql' + # import UserFields from 'user-fields.fragment.graphql' query { - foo { - ...FooFields + user { + ...UserFields } } `, @@ -69,7 +69,6 @@ export const rule: GraphQLESLintRule = { schema: [], }, create(context) { - const definedFragments = new Set(); const fragmentSpreadNameNodes = new Set>(); const comments = context.getSourceCode().getAllComments(); const siblings = requireSiblingsOperations(RULE_ID, context); @@ -78,10 +77,6 @@ export const rule: GraphQLESLintRule = { function checkFragmentSpreadNameNode(node: GraphQLESTreeNode): void { const fragmentName = node.value; - if (definedFragments.has(fragmentName)) { - return; - } - const fragmentsFromSiblings = siblings.getFragment(fragmentName); for (const comment of comments) { @@ -99,29 +94,30 @@ export const rule: GraphQLESLintRule = { const importPath = path.join(path.dirname(filePath), extractedImportPath); - const hasInSiblings = fragmentsFromSiblings.some(source => source.filePath === importPath); + const hasInSiblings = fragmentsFromSiblings.some(source => importPath === source.filePath); if (hasInSiblings) return; } + const fragmentInSameFile = fragmentsFromSiblings.some(source => source.filePath === filePath) + + if (fragmentInSameFile) return; + context.report({ node, messageId: RULE_ID, data: { fragmentName }, - suggest: [ - { - messageId: SUGGESTION_ID, - data: { fragmentName }, - fix(fixer) { - const suggestedPath = fragmentsFromSiblings.length - ? path.relative(path.dirname(filePath), fragmentsFromSiblings[0].filePath) - : 'CHANGE_ME.graphql'; - return fixer.insertTextBeforeRange( - [0, 0], - `# import ${fragmentName} from '${suggestedPath}'\n`, - ); - }, - }, - ], + suggest: (fragmentsFromSiblings.length + ? fragmentsFromSiblings.map(o => path.relative(path.dirname(filePath), o.filePath)) + : ['CHANGE_ME.graphql'] + ).map(suggestedPath => ({ + messageId: SUGGESTION_ID, + data: { fragmentName }, + fix: fixer => + fixer.insertTextBeforeRange( + [0, 0], + `# import ${fragmentName} from '${suggestedPath}'\n`, + ), + })), }); } @@ -129,9 +125,6 @@ export const rule: GraphQLESLintRule = { 'FragmentSpread .name'(node: GraphQLESTreeNode) { fragmentSpreadNameNodes.add(node); }, - 'FragmentDefinition .name'(node: GraphQLESTreeNode) { - definedFragments.add(node.value); - }, 'Document:exit'() { for (const fragmentSpreadNameNode of fragmentSpreadNameNodes) { checkFragmentSpreadNameNode(fragmentSpreadNameNode); diff --git a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md index f4a2cdd2c4a..8f667b2372d 100644 --- a/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md +++ b/packages/plugin/tests/__snapshots__/require-import-fragment.spec.md @@ -3,27 +3,27 @@ exports[`should report fragments when there are no import expressions 1`] = ` #### ⌨️ Code - 1 | { - 2 | foo { - 3 | ...FooFields - 4 | } - 5 | } + 1 | { + 2 | foo { + 3 | ...FooFields + 4 | } + 5 | } #### ❌ Error - 2 | foo { - > 3 | ...FooFields - | ^^^^^^^^^ Expected "FooFields" fragment to be imported. - 4 | } + 2 | foo { + > 3 | ...FooFields + | ^^^^^^^^^ Expected "FooFields" fragment to be imported. + 4 | } #### 💡 Suggestion: Add import expression for "FooFields". - 1 | # import FooFields from 'CHANGE_ME.graphql' - 2 | { - 3 | foo { - 4 | ...FooFields - 5 | } - 6 | } + 1 | # import FooFields from 'foo-fragment.gql' + 2 | { + 3 | foo { + 4 | ...FooFields + 5 | } + 6 | } `; exports[`should report with default import 1`] = ` diff --git a/packages/plugin/tests/mocks/import-fragments/missing-import.gql b/packages/plugin/tests/mocks/import-fragments/missing-import.gql new file mode 100644 index 00000000000..9563590e406 --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/missing-import.gql @@ -0,0 +1,5 @@ +{ + foo { + ...FooFields + } +} diff --git a/packages/plugin/tests/mocks/import-fragments/same-file.gql b/packages/plugin/tests/mocks/import-fragments/same-file.gql new file mode 100644 index 00000000000..77c4b74b223 --- /dev/null +++ b/packages/plugin/tests/mocks/import-fragments/same-file.gql @@ -0,0 +1,9 @@ +{ + foo { + ...FooFields + } +} + +fragment FooFields on Foo { + id +} diff --git a/packages/plugin/tests/require-import-fragment.spec.ts b/packages/plugin/tests/require-import-fragment.spec.ts index 671d5786eba..7c6b4690680 100644 --- a/packages/plugin/tests/require-import-fragment.spec.ts +++ b/packages/plugin/tests/require-import-fragment.spec.ts @@ -4,15 +4,6 @@ import { rule } from '../src/rules/require-import-fragment'; const ruleTester = new GraphQLRuleTester(); -function withSiblings(code: string) { - return { - code, - parserOptions: { - documents: code, - }, - }; -} - function withMocks({ name, filename }: { name: string; filename: string }) { return { name, @@ -20,7 +11,7 @@ function withMocks({ name, filename }: { name: string; filename: string }) { code: ruleTester.fromMockFile(filename.split('/mocks')[1]), parserOptions: { documents: [ - filename, + filename, join(__dirname, 'mocks/import-fragments/foo-fragment.gql'), join(__dirname, 'mocks/import-fragments/bar-fragment.gql'), ], @@ -38,20 +29,10 @@ ruleTester.runGraphQLTests('require-import-fragment', rule, { name: 'should not report with default import', filename: join(__dirname, 'mocks/import-fragments/valid-query-default.gql'), }), - { + withMocks({ name: 'should not report fragments from the same file', - ...withSiblings(/* GraphQL */ ` - { - foo { - ...FooFields - } - } - - fragment FooFields on Foo { - id - } - `), - }, + filename: join(__dirname, 'mocks/import-fragments/same-file.gql'), + }), ], invalid: [ { @@ -69,14 +50,10 @@ ruleTester.runGraphQLTests('require-import-fragment', rule, { errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], }, { - name: 'should report fragments when there are no import expressions', - ...withSiblings(/* GraphQL */ ` - { - foo { - ...FooFields - } - } - `), + ...withMocks({ + name: 'should report fragments when there are no import expressions', + filename: join(__dirname, 'mocks/import-fragments/missing-import.gql'), + }), errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], }, ], diff --git a/website/src/pages/rules/require-import-fragment.md b/website/src/pages/rules/require-import-fragment.md index 559394fe19c..a87b4e9647d 100644 --- a/website/src/pages/rules/require-import-fragment.md +++ b/website/src/pages/rules/require-import-fragment.md @@ -20,8 +20,8 @@ Require fragments to be imported via an import expression. # eslint @graphql-eslint/require-import-fragment: 'error' query { - foo { - ...FooFields + user { + ...UserFields } } ``` @@ -31,10 +31,10 @@ query { ```graphql # eslint @graphql-eslint/require-import-fragment: 'error' -# import 'bar.graphql' +# import 'post-fields.fragment.graphql' query { - foo { - ...FooFields + user { + ...UserFields } } ``` @@ -44,10 +44,10 @@ query { ```graphql # eslint @graphql-eslint/require-import-fragment: 'error' -# import FooFields from 'bar.graphql' +# import UserFields from 'post-fields.fragment.graphql' query { - foo { - ...FooFields + user { + ...UserFields } } ``` @@ -57,10 +57,10 @@ query { ```graphql # eslint @graphql-eslint/require-import-fragment: 'error' -# import FooFields from 'foo.graphql' +# import UserFields from 'user-fields.fragment.graphql' query { - foo { - ...FooFields + user { + ...UserFields } } ``` From 65f0a4558db57c2eeae63ea1b1a8debc5e83afe0 Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Wed, 8 Feb 2023 20:24:33 +0100 Subject: [PATCH 13/15] prettify --- packages/plugin/src/rules/require-import-fragment.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index ca9e796c115..c3b15ce94b0 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -76,7 +76,6 @@ export const rule: GraphQLESLintRule = { function checkFragmentSpreadNameNode(node: GraphQLESTreeNode): void { const fragmentName = node.value; - const fragmentsFromSiblings = siblings.getFragment(fragmentName); for (const comment of comments) { @@ -93,13 +92,11 @@ export const rule: GraphQLESLintRule = { if (!extractedImportPath) continue; const importPath = path.join(path.dirname(filePath), extractedImportPath); - const hasInSiblings = fragmentsFromSiblings.some(source => importPath === source.filePath); if (hasInSiblings) return; } - const fragmentInSameFile = fragmentsFromSiblings.some(source => source.filePath === filePath) - + const fragmentInSameFile = fragmentsFromSiblings.some(source => source.filePath === filePath); if (fragmentInSameFile) return; context.report({ From 16606175f775b7401b9990f22de4e834103961e6 Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Wed, 8 Feb 2023 21:16:58 +0100 Subject: [PATCH 14/15] simplify --- .../src/rules/require-import-fragment.ts | 86 +++++++++---------- packages/plugin/src/testkit.ts | 4 +- .../tests/require-import-fragment.spec.ts | 53 +++++++----- 3 files changed, 75 insertions(+), 68 deletions(-) diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index c3b15ce94b0..7ba551cf815 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -69,63 +69,57 @@ export const rule: GraphQLESLintRule = { schema: [], }, create(context) { - const fragmentSpreadNameNodes = new Set>(); const comments = context.getSourceCode().getAllComments(); const siblings = requireSiblingsOperations(RULE_ID, context); const filePath = context.getFilename(); - function checkFragmentSpreadNameNode(node: GraphQLESTreeNode): void { - const fragmentName = node.value; - const fragmentsFromSiblings = siblings.getFragment(fragmentName); + return { + 'FragmentSpread .name'(node: GraphQLESTreeNode) { + const fragmentName = node.value; + const fragmentsFromSiblings = siblings.getFragment(fragmentName); - for (const comment of comments) { - if (comment.type !== 'Line') continue; + for (const comment of comments) { + if (comment.type !== 'Line') continue; - // 1. could start with extra whitespace - // 2. match both named/default import - const isPossibleImported = new RegExp( - `^\\s*import\\s+(${fragmentName}\\s+from\\s+)?['"]`, - ).test(comment.value); - if (!isPossibleImported) continue; + // 1. could start with extra whitespace + // 2. match both named/default import + const isPossibleImported = new RegExp( + `^\\s*import\\s+(${fragmentName}\\s+from\\s+)?['"]`, + ).test(comment.value); + if (!isPossibleImported) continue; - const extractedImportPath = comment.value.match(/(["'])((?:\1|.)*?)\1/)?.[2]; - if (!extractedImportPath) continue; + const extractedImportPath = comment.value.match(/(["'])((?:\1|.)*?)\1/)?.[2]; + if (!extractedImportPath) continue; - const importPath = path.join(path.dirname(filePath), extractedImportPath); - const hasInSiblings = fragmentsFromSiblings.some(source => importPath === source.filePath); - if (hasInSiblings) return; - } + const importPath = path.join(path.dirname(filePath), extractedImportPath); + const hasInSiblings = fragmentsFromSiblings.some( + source => importPath === source.filePath, + ); + if (hasInSiblings) return; + } - const fragmentInSameFile = fragmentsFromSiblings.some(source => source.filePath === filePath); - if (fragmentInSameFile) return; + const fragmentInSameFile = fragmentsFromSiblings.some( + source => source.filePath === filePath, + ); + if (fragmentInSameFile) return; - context.report({ - node, - messageId: RULE_ID, - data: { fragmentName }, - suggest: (fragmentsFromSiblings.length - ? fragmentsFromSiblings.map(o => path.relative(path.dirname(filePath), o.filePath)) - : ['CHANGE_ME.graphql'] - ).map(suggestedPath => ({ - messageId: SUGGESTION_ID, + context.report({ + node, + messageId: RULE_ID, data: { fragmentName }, - fix: fixer => - fixer.insertTextBeforeRange( - [0, 0], - `# import ${fragmentName} from '${suggestedPath}'\n`, - ), - })), - }); - } - - return { - 'FragmentSpread .name'(node: GraphQLESTreeNode) { - fragmentSpreadNameNodes.add(node); - }, - 'Document:exit'() { - for (const fragmentSpreadNameNode of fragmentSpreadNameNodes) { - checkFragmentSpreadNameNode(fragmentSpreadNameNode); - } + suggest: (fragmentsFromSiblings.length + ? fragmentsFromSiblings.map(o => path.relative(path.dirname(filePath), o.filePath)) + : ['CHANGE_ME.graphql'] + ).map(suggestedPath => ({ + messageId: SUGGESTION_ID, + data: { fragmentName }, + fix: fixer => + fixer.insertTextBeforeRange( + [0, 0], + `# import ${fragmentName} from '${suggestedPath}'\n`, + ), + })), + }); }, }; }, diff --git a/packages/plugin/src/testkit.ts b/packages/plugin/src/testkit.ts index 22f0add9311..84cf444da59 100644 --- a/packages/plugin/src/testkit.ts +++ b/packages/plugin/src/testkit.ts @@ -13,7 +13,7 @@ export type GraphQLESLintRuleListener = Re [K in keyof ASTKindToNode]?: (node: GraphQLESTreeNode) => void; }; -export type GraphQLValidTestCase = Omit< +export type GraphQLValidTestCase = Omit< RuleTester.ValidTestCase, 'options' | 'parserOptions' > & { @@ -21,7 +21,7 @@ export type GraphQLValidTestCase = Omit< parserOptions?: Omit; }; -export type GraphQLInvalidTestCase = GraphQLValidTestCase & { +export type GraphQLInvalidTestCase = GraphQLValidTestCase & { errors: (RuleTester.TestCaseError | string)[] | number; output?: string | null; }; diff --git a/packages/plugin/tests/require-import-fragment.spec.ts b/packages/plugin/tests/require-import-fragment.spec.ts index 7c6b4690680..4a09f908372 100644 --- a/packages/plugin/tests/require-import-fragment.spec.ts +++ b/packages/plugin/tests/require-import-fragment.spec.ts @@ -1,10 +1,28 @@ import { join } from 'node:path'; -import { GraphQLRuleTester } from '../src'; +import { GraphQLInvalidTestCase, GraphQLRuleTester } from '../src'; import { rule } from '../src/rules/require-import-fragment'; +import { Linter } from 'eslint'; +import ParserOptions = Linter.ParserOptions; const ruleTester = new GraphQLRuleTester(); -function withMocks({ name, filename }: { name: string; filename: string }) { +function withMocks({ + name, + filename, + errors, +}: { + name: string; + filename: string; + errors?: GraphQLInvalidTestCase['errors']; +}): { + name: string; + filename: string; + code: string; + parserOptions: { + documents: ParserOptions['documents']; + }; + errors: any; +} { return { name, filename, @@ -16,6 +34,7 @@ function withMocks({ name, filename }: { name: string; filename: string }) { join(__dirname, 'mocks/import-fragments/bar-fragment.gql'), ], }, + errors, }; } @@ -35,26 +54,20 @@ ruleTester.runGraphQLTests('require-import-fragment', rule, { }), ], invalid: [ - { - ...withMocks({ - name: 'should report with named import', - filename: join(__dirname, 'mocks/import-fragments/invalid-query.gql'), - }), + withMocks({ + name: 'should report with named import', + filename: join(__dirname, 'mocks/import-fragments/invalid-query.gql'), errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], - }, - { - ...withMocks({ - name: 'should report with default import', - filename: join(__dirname, 'mocks/import-fragments/invalid-query-default.gql'), - }), + }), + withMocks({ + name: 'should report with default import', + filename: join(__dirname, 'mocks/import-fragments/invalid-query-default.gql'), errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], - }, - { - ...withMocks({ - name: 'should report fragments when there are no import expressions', - filename: join(__dirname, 'mocks/import-fragments/missing-import.gql'), - }), + }), + withMocks({ + name: 'should report fragments when there are no import expressions', + filename: join(__dirname, 'mocks/import-fragments/missing-import.gql'), errors: [{ message: 'Expected "FooFields" fragment to be imported.' }], - }, + }), ], }); From 007cbf38cdf83cd4108035b564ae12a5125a1608 Mon Sep 17 00:00:00 2001 From: Dimitri POSTOLOV Date: Thu, 9 Feb 2023 01:20:44 +0100 Subject: [PATCH 15/15] last refactor --- packages/plugin/src/configs/operations-all.ts | 1 - .../plugin/src/rules/require-import-fragment.ts | 14 ++++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/plugin/src/configs/operations-all.ts b/packages/plugin/src/configs/operations-all.ts index c2dbb69a47c..cdf9e59b0f2 100644 --- a/packages/plugin/src/configs/operations-all.ts +++ b/packages/plugin/src/configs/operations-all.ts @@ -24,7 +24,6 @@ export default { }, ], '@graphql-eslint/no-one-place-fragments': 'error', - '@graphql-eslint/require-import-fragment': 'error', '@graphql-eslint/unique-fragment-name': 'error', '@graphql-eslint/unique-operation-name': 'error', }, diff --git a/packages/plugin/src/rules/require-import-fragment.ts b/packages/plugin/src/rules/require-import-fragment.ts index 7ba551cf815..94df25fd633 100644 --- a/packages/plugin/src/rules/require-import-fragment.ts +++ b/packages/plugin/src/rules/require-import-fragment.ts @@ -60,6 +60,7 @@ export const rule: GraphQLESLintRule = { }, ], requiresSiblings: true, + isDisabledForAllConfig: true, }, hasSuggestions: true, messages: { @@ -74,7 +75,7 @@ export const rule: GraphQLESLintRule = { const filePath = context.getFilename(); return { - 'FragmentSpread .name'(node: GraphQLESTreeNode) { + 'FragmentSpread > .name'(node: GraphQLESTreeNode) { const fragmentName = node.value; const fragmentsFromSiblings = siblings.getFragment(fragmentName); @@ -93,7 +94,7 @@ export const rule: GraphQLESLintRule = { const importPath = path.join(path.dirname(filePath), extractedImportPath); const hasInSiblings = fragmentsFromSiblings.some( - source => importPath === source.filePath, + source => source.filePath === importPath, ); if (hasInSiblings) return; } @@ -103,14 +104,15 @@ export const rule: GraphQLESLintRule = { ); if (fragmentInSameFile) return; + const suggestedFilePaths = fragmentsFromSiblings.length + ? fragmentsFromSiblings.map(o => path.relative(path.dirname(filePath), o.filePath)) + : ['CHANGE_ME.graphql']; + context.report({ node, messageId: RULE_ID, data: { fragmentName }, - suggest: (fragmentsFromSiblings.length - ? fragmentsFromSiblings.map(o => path.relative(path.dirname(filePath), o.filePath)) - : ['CHANGE_ME.graphql'] - ).map(suggestedPath => ({ + suggest: suggestedFilePaths.map(suggestedPath => ({ messageId: SUGGESTION_ID, data: { fragmentName }, fix: fixer =>