2️⃣ Fix unique-fragment-name and no-unused-fragments rules, skip graphql #import comments for siblings (#511)

Author: Dimitri POSTOLOV

.changeset/witty-beans-turn.md

@@ -0,0 +1,5 @@
+---
+'@graphql-eslint/eslint-plugin': patch
+---
+
+fix unique-fragment-name and no-unused-fragments rule

docs/rules/no-unused-fragments.md

@@ -3,7 +3,7 @@
 - Category: `Validation`
 - Rule name: `@graphql-eslint/no-unused-fragments`
 - Requires GraphQL Schema: `true` [ℹ️](../../README.md#extended-linting-rules-with-graphql-schema)
-- Requires GraphQL Operations: `false` [ℹ️](../../README.md#extended-linting-rules-with-siblings-operations)
+- Requires GraphQL Operations: `true` [ℹ️](../../README.md#extended-linting-rules-with-siblings-operations)
 
 A GraphQL document is only valid if all fragment definitions are spread within operations, or spread within other fragments spread within operations.
 

packages/plugin/package.json

@@ -16,22 +16,23 @@
     "prepack": "bob prepack"
   },
   "dependencies": {
-    "@graphql-tools/utils": "~7.10.0",
-    "@graphql-tools/load": "~6.2.0",
+    "@graphql-tools/code-file-loader": "~6.3.0",
     "@graphql-tools/graphql-file-loader": "~6.2.0",
+    "@graphql-tools/graphql-tag-pluck": "~6.5.0",
+    "@graphql-tools/import": "^6.3.1",
     "@graphql-tools/json-file-loader": "~6.2.6",
-    "@graphql-tools/code-file-loader": "~6.3.0",
+    "@graphql-tools/load": "~6.2.0",
     "@graphql-tools/url-loader": "~6.10.0",
-    "@graphql-tools/graphql-tag-pluck": "~6.5.0",
+    "@graphql-tools/utils": "~7.10.0",
     "graphql-config": "^3.2.0",
     "graphql-depth-limit": "1.1.0"
   },
   "devDependencies": {
-    "@types/graphql-depth-limit": "1.1.2",
     "@types/eslint": "7.2.13",
+    "@types/graphql-depth-limit": "1.1.2",
     "bob-the-bundler": "1.2.1",
-    "graphql-config": "3.3.0",
     "graphql": "15.5.1",
+    "graphql-config": "3.3.0",
     "typescript": "4.3.5"
   },
   "peerDependencies": {

packages/plugin/src/rules/graphql-js-validation.ts

@@ -1,9 +1,10 @@
 import { validate, GraphQLSchema, DocumentNode, ASTNode, ValidationRule } from 'graphql';
 import { validateSDL } from 'graphql/validation/validate';
-import { GraphQLFileLoader } from '@graphql-tools/graphql-file-loader';
-import fs from 'fs';
+import { parseImportLine, processImport } from '@graphql-tools/import';
+import { existsSync } from 'fs';
+import { join, dirname } from 'path';
 import { GraphQLESLintRule, GraphQLESLintRuleContext } from '../types';
-import { requireGraphQLSchemaFromContext } from '../utils';
+import { requireGraphQLSchemaFromContext, requireSiblingsOperations } from '../utils';
 import { GraphQLESTreeNode } from '../estree-parser';
 
 function extractRuleName(stack: string | undefined): string | null {
@@ -24,7 +25,7 @@ export function validateDoc(
   rules: ReadonlyArray<ValidationRule>,
   ruleName: string | null = null
 ): void {
-  if (documentNode && documentNode.definitions && documentNode.definitions.length > 0) {
+  if (documentNode?.definitions?.length > 0) {
     try {
       const validationErrors = schema ? validate(schema, documentNode, rules) : validateSDL(documentNode, null, rules);
 
@@ -69,15 +70,15 @@ const validationToRule = (
   }
 
   const requiresSchema = docs.requiresSchema ?? true;
-
+  const requiresSiblings = docs.requiresSiblings ?? false;
   return {
     [name]: {
       meta: {
         docs: {
           ...docs,
           category: 'Validation',
           requiresSchema,
-          requiresSiblings: false,
+          requiresSiblings,
           url: `https://github.com/dotansimha/graphql-eslint/blob/master/docs/rules/${name}.md`,
           description: `${docs.description}\n\n> This rule is a wrapper around a \`graphql-js\` validation function. [You can find it's source code here](https://github.com/graphql/graphql-js/blob/main/src/validation/rules/${ruleName}Rule.ts).`,
         },
@@ -96,9 +97,8 @@ const validationToRule = (
             const schema = requiresSchema ? requireGraphQLSchemaFromContext(name, context) : null;
 
             let documentNode: DocumentNode;
-            const filePath = context.getFilename();
-            const isVirtualFile = !fs.existsSync(filePath);
-            if (!isVirtualFile && getDocumentNode) {
+            const isRealFile = existsSync(context.getFilename());
+            if (isRealFile && getDocumentNode) {
               documentNode = getDocumentNode(context);
             }
             validateDoc(node, context, schema, documentNode || node.rawNode(), [ruleFn], ruleName);
@@ -180,10 +180,8 @@ export const GRAPHQL_JS_VALIDATIONS = Object.assign(
       if (!isGraphQLImportFile(code)) {
         return null;
       }
-      // Import documents if file contains '#import' comments
-      const fileLoader = new GraphQLFileLoader();
-      const graphqlAst = fileLoader.handleFileContent(code, context.getFilename(), { noLocation: true });
-      return graphqlAst.document;
+      // Import documents because file contains '#import' comments
+      return processImport(context.getFilename());
     }
   ),
   validationToRule('known-type-names', 'KnownTypeNames', {
@@ -202,9 +200,45 @@ export const GRAPHQL_JS_VALIDATIONS = Object.assign(
   validationToRule('no-undefined-variables', 'NoUndefinedVariables', {
     description: `A GraphQL operation is only valid if all variables encountered, both directly and via fragment spreads, are defined by that operation.`,
   }),
-  validationToRule('no-unused-fragments', 'NoUnusedFragments', {
-    description: `A GraphQL document is only valid if all fragment definitions are spread within operations, or spread within other fragments spread within operations.`,
-  }),
+  validationToRule(
+    'no-unused-fragments',
+    'NoUnusedFragments',
+    {
+      description: `A GraphQL document is only valid if all fragment definitions are spread within operations, or spread within other fragments spread within operations.`,
+      requiresSiblings: true,
+    },
+    context => {
+      const siblings = requireSiblingsOperations('no-unused-fragments', context);
+      const documents = [...siblings.getOperations(), ...siblings.getFragments()]
+        .filter(({ document }) => isGraphQLImportFile(document.loc.source.body))
+        .map(({ filePath, document }) => ({
+          filePath,
+          code: document.loc.source.body,
+        }));
+
+      const getParentNode = (filePath: string): DocumentNode | null => {
+        for (const { filePath: docFilePath, code } of documents) {
+          const isFileImported = code
+            .split('\n')
+            .filter(isGraphQLImportFile)
+            .map(line => parseImportLine(line.replace('#', '')))
+            .some(o => filePath === join(dirname(docFilePath), o.from));
+
+          if (!isFileImported) {
+            continue;
+          }
+          // Import first file that import this file
+          const document = processImport(docFilePath);
+          // Import most top file that import this file
+          return getParentNode(docFilePath) || document;
+        }
+
+        return null;
+      };
+
+      return getParentNode(context.getFilename());
+    }
+  ),
   validationToRule('no-unused-variables', 'NoUnusedVariables', {
     description: `A GraphQL operation is only valid if all variables defined by an operation are used, either directly or within a spread fragment.`,
   }),

packages/plugin/src/rules/unique-fragment-name.ts

@@ -1,3 +1,4 @@
+import { relative } from 'path';
 import { GraphQLESLintRule } from '../types';
 import { normalizePath, requireSiblingsOperations } from '../utils';
 
@@ -51,19 +52,20 @@ const rule: GraphQLESLintRule<[], false> = {
     },
   },
   create(context) {
+    const filename = context.getFilename();
     return {
-      FragmentDefinition: node => {
+      FragmentDefinition(node) {
         const fragmentName = node.name?.value;
         const siblings = requireSiblingsOperations('unique-fragment-name', context);
 
         if (fragmentName) {
           const siblingFragments = siblings.getFragment(fragmentName);
+          const conflictingFragments = siblingFragments.filter(f => {
+            const isSameName = f.document.name?.value === fragmentName;
+            const isSamePath = normalizePath(f.filePath) === normalizePath(filename);
 
-          const conflictingFragments = siblingFragments.filter(
-            f =>
-              f.document.name?.value === fragmentName &&
-              normalizePath(f.filePath) !== normalizePath(context.getFilename())
-          );
+            return isSameName && !isSamePath;
+          });
 
           if (conflictingFragments.length > 0) {
             context.report({
@@ -80,7 +82,7 @@ const rule: GraphQLESLintRule<[], false> = {
               messageId: UNIQUE_FRAGMENT_NAME,
               data: {
                 fragmentName,
-                fragmentsSummary: conflictingFragments.map(f => `\t${f.filePath}`).join('\n'),
+                fragmentsSummary: conflictingFragments.map(f => `\t${relative(process.cwd(), f.filePath)}`).join('\n'),
               },
             });
           }

packages/plugin/src/sibling-operations.ts

@@ -50,6 +50,7 @@ function loadSiblings(baseDir: string, loadPaths: string[]): Source[] {
   return loadDocumentsSync(loadPaths, {
     cwd: baseDir,
     loaders: operationsLoaders,
+    skipGraphQLImport: true,
   });
 }
 
@@ -69,8 +70,10 @@ export function getSiblingOperations(options: ParserOptions, gqlConfig: GraphQLC
     } else {
       const projectForFile = gqlConfig.getProjectForFile(options.filePath);
 
-      if (projectForFile) {
-        siblings = projectForFile.getDocumentsSync();
+      if (projectForFile && projectForFile.documents.length > 0) {
+        siblings = projectForFile.loadDocumentsSync(projectForFile.documents, {
+          skipGraphQLImport: true,
+        });
         operationsCache.set(fileDir, siblings);
       }
     }

packages/plugin/tests/known-fragment-names.spec.ts

@@ -2,17 +2,6 @@ import { join } from 'path';
 import { GraphQLRuleTester } from '../src';
 import { GRAPHQL_JS_VALIDATIONS } from '../src/rules/graphql-js-validation';
 
-const TEST_SCHEMA = /* GraphQL */ `
-  type User {
-    id: ID!
-    firstName: String!
-  }
-
-  type Query {
-    user: User
-  }
-`;
-
 const ruleTester = new GraphQLRuleTester();
 
 ruleTester.runGraphQLTests('known-fragment-names', GRAPHQL_JS_VALIDATIONS['known-fragment-names'], {
@@ -21,7 +10,7 @@ ruleTester.runGraphQLTests('known-fragment-names', GRAPHQL_JS_VALIDATIONS['known
       filename: join(__dirname, 'mocks/user.graphql'),
       code: ruleTester.fromMockFile('user.graphql'),
       parserOptions: {
-        schema: TEST_SCHEMA,
+        schema: join(__dirname, 'mocks/user-schema.graphql'),
       },
     },
   ],

packages/plugin/tests/mocks/post-fields.graphql

@@ -0,0 +1,7 @@
+#import './user-fields.graphql'
+
+fragment PostFields on Post {
+  user {
+    ...UserFields
+  }
+}

packages/plugin/tests/mocks/post.graphql

@@ -0,0 +1,7 @@
+#import "./post-fields.graphql"
+
+query Post {
+  post {
+    ...PostFields
+  }
+}

packages/plugin/tests/mocks/user-schema.graphql

@@ -0,0 +1,14 @@
+type User {
+  id: ID!
+  firstName: String!
+}
+
+type Post {
+  id: ID!
+  user: User!
+}
+
+type Query {
+  user: User
+  post: Post
+}