fix linting in query and variable editor (#2427)

* fix linting in query editor * add e2e test suite for query and variable linting * fix error message assertion based on GraphQL version
graphql · May 23, 2022 · ebc864f · ebc864f
1 parent 90ec49a
commit ebc864f
Show file tree

Hide file tree

Showing 7 changed files with 193 additions and 1 deletion.
diff --git a/.changeset/olive-cooks-juggle.md b/.changeset/olive-cooks-juggle.md
@@ -0,0 +1,5 @@
+---
+'@graphiql/react': patch
+---
+
+Mark `graphql` as external dependency to avoid importing multiple instances
diff --git a/.changeset/orange-clocks-march.md b/.changeset/orange-clocks-march.md
@@ -0,0 +1,5 @@
+---
+'@graphiql/react': patch
+---
+
+Fix linting by also updating the options object in the internal codemirror state
diff --git a/packages/graphiql-react/src/editor/query-editor.tsx b/packages/graphiql-react/src/editor/query-editor.tsx
@@ -235,6 +235,7 @@ function useSynchronizeSchema(
 
     const didChange = editor.options.lint.schema !== schema;
 
+    editor.state.lint.linterOptions.schema = schema;
     editor.options.lint.schema = schema;
     editor.options.hintOptions.schema = schema;
     editor.options.info.schema = schema;
@@ -258,6 +259,7 @@ function useSynchronizeValidationRules(
 
     const didChange = editor.options.lint.validationRules !== validationRules;
 
+    editor.state.lint.linterOptions.validationRules = validationRules;
     editor.options.lint.validationRules = validationRules;
 
     if (didChange && codeMirrorRef.current) {
@@ -279,6 +281,7 @@ function useSynchronizeExternalFragments(
     const didChange =
       editor.options.lint.externalFragments !== externalFragments;
 
+    editor.state.lint.linterOptions.externalFragments = externalFragments;
     editor.options.lint.externalFragments = externalFragments;
     editor.options.hintOptions.externalFragments = externalFragments;
 

diff --git a/packages/graphiql-react/src/editor/variable-editor.tsx b/packages/graphiql-react/src/editor/variable-editor.tsx
@@ -161,6 +161,7 @@ function useSynchronizeVariableTypes(
 
     const didChange = editor.options.lint.variableToType !== variableToType;
 
+    editor.state.lint.linterOptions.variableToType = variableToType;
     editor.options.lint.variableToType = variableToType;
     editor.options.hintOptions.variableToType = variableToType;
 

diff --git a/packages/graphiql-react/vite.config.ts b/packages/graphiql-react/vite.config.ts
@@ -10,7 +10,7 @@ export default defineConfig({
       formats: ['cjs', 'es'],
     },
     rollupOptions: {
-      external: ['react', 'react-dom'],
+      external: ['graphql', 'react', 'react-dom'],
       output: {
         chunkFileNames: '[name].[format].js',
       },

diff --git a/packages/graphiql/cypress/integration/lint.spec.ts b/packages/graphiql/cypress/integration/lint.spec.ts
@@ -0,0 +1,160 @@
+import { version as graphqlVersion } from 'graphql/version';
+
+describe('Linting', () => {
+  it('Marks GraphQL syntax errors as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        {
+          doesNotExist
+          test {
+            id
+          }
+          +++
+        }
+      `,
+    }).assertLinterMarkWithMessage(
+      '+++',
+      'error',
+      graphqlVersion.startsWith('15.')
+        ? 'Syntax Error: Cannot parse the unexpected character "+".'
+        : 'Syntax Error: Unexpected character: "+".',
+    );
+  });
+
+  it('Does not mark valid fields', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        {
+          myAlias: id
+          test {
+            id
+          }
+        }
+      `,
+    })
+      .contains('myAlias')
+      .should('not.have.class', 'CodeMirror-lint-mark')
+      .and('not.have.class', 'CodeMirror-lint-mark-error');
+  });
+
+  it('Marks invalid fields as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        {
+          doesNotExist
+          test {
+            id
+          }
+        }
+      `,
+    }).assertLinterMarkWithMessage(
+      'doesNotExist',
+      'error',
+      'Cannot query field "doesNotExist" on type "Test".',
+    );
+  });
+
+  it('Marks deprecated fields as warning', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        {
+          id
+          deprecatedField {
+            id
+          }
+        }
+      `,
+    }).assertLinterMarkWithMessage(
+      'deprecatedField',
+      'warning',
+      'The field Test.deprecatedField is deprecated.',
+    );
+  });
+
+  it('Marks syntax errors in variables JSON as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        query WithVariables($stringArg: String) {
+          hasArgs(string: $stringArg)
+        }
+      `,
+      variablesString: JSON.stringify({ stringArg: '42' }, null, 2).slice(
+        0,
+        -1,
+      ),
+    }).assertLinterMarkWithMessage(
+      '"42"',
+      'error',
+      'Expected } but found [end of file].',
+    );
+  });
+
+  it('Marks unused variables as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        query WithVariables($stringArg: String) {
+          hasArgs(string: $stringArg)
+        }
+      `,
+      variables: {
+        stringArg: '42',
+        unusedVariable: 'whoops',
+      },
+    }).assertLinterMarkWithMessage(
+      'unusedVariable',
+      'error',
+      'Variable "$unusedVariable" does not appear in any GraphQL query.',
+    );
+  });
+
+  it('Marks invalid variable type as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        query WithVariables($stringArg: String) {
+          hasArgs(string: $stringArg)
+        }
+      `,
+      variables: {
+        stringArg: 42,
+      },
+    }).assertLinterMarkWithMessage(
+      '42',
+      'error',
+      'Expected value of type "String".',
+    );
+  });
+
+  it('Marks variables with null values for a non-nullable type as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        query WithVariables($stringArg: String!) {
+          hasArgs(string: $stringArg)
+        }
+      `,
+      variables: {
+        stringArg: null,
+      },
+    }).assertLinterMarkWithMessage(
+      'null',
+      'error',
+      'Type "String!" is non-nullable and cannot be null.',
+    );
+  });
+
+  it('Marks variables with non-object values for a input object type as error', () => {
+    cy.visitWithOp({
+      query: /* GraphQL */ `
+        query WithVariables($objectArg: TestInput) {
+          hasArgs(object: $objectArg)
+        }
+      `,
+      variables: {
+        objectArg: '42',
+      },
+    }).assertLinterMarkWithMessage(
+      '"42"',
+      'error',
+      'Type "TestInput" must be an Object.',
+    );
+  });
+});
diff --git a/packages/graphiql/cypress/support/commands.ts b/packages/graphiql/cypress/support/commands.ts
@@ -37,6 +37,11 @@ declare namespace Cypress {
       expectedResult: MockResult,
       timeout?: number,
     ): Chainable<Element>;
+    assertLinterMarkWithMessage(
+      text: string,
+      severity: 'error' | 'warning',
+      message?: string,
+    ): Chainable<Element>;
   }
 }
 
@@ -102,3 +107,16 @@ function codeWithLineNumbers(code: string): string {
     .map((line, i) => `${i + 1}\n${line}`)
     .join('\n');
 }
+
+Cypress.Commands.add(
+  'assertLinterMarkWithMessage',
+  (text, severity, message) => {
+    cy.contains(text)
+      .should('have.class', 'CodeMirror-lint-mark')
+      .and('have.class', `CodeMirror-lint-mark-${severity}`);
+    if (message) {
+      cy.contains(text).trigger('mouseover');
+      cy.contains(message);
+    }
+  },
+);