Lint rule to make sure queries requiring QueryVariables have them (#8102

)

* lint rule

* revert unecessary change

* no existential operator

* merge conflict

* lint

* prettier

* added fixer

* check if variables is already imported

js_modules/dagit/packages/eslint-config/.prettierrc.js

@@ -2,5 +2,5 @@ module.exports = {
   bracketSpacing: false,
   printWidth: 100,
   singleQuote: true,
-  trailingComma: 'all'
+  trailingComma: 'all',
 };

js_modules/dagit/packages/eslint-config/index.js

@@ -1,6 +1,7 @@
 module.exports = {
   parser: '@typescript-eslint/parser',
   extends: [
+    'plugin:eslint-plugin-dagster-rules/all',
     'plugin:react/recommended',
     'plugin:jest/recommended',
     'plugin:@typescript-eslint/recommended',

js_modules/dagit/packages/eslint-config/package.json

@@ -5,7 +5,9 @@
   "license": "Apache-2.0",
   "main": "index.js",
   "scripts": {
-    "lint": "eslint . --ext=.tsx,.ts,.js --fix -c .eslintrc.js"
+    "lint": "eslint . --ext=.tsx,.ts,.js --fix -c .eslintrc.js",
+    "test": "jest",
+    "format": "prettier -w --config ./.prettierrc.js ."
   },
   "peerDependencies": {
     "eslint": "^8.6.0",
@@ -15,6 +17,7 @@
     "@typescript-eslint/eslint-plugin": "5.21.0",
     "@typescript-eslint/parser": "5.21.0",
     "eslint-config-prettier": "8.5.0",
+    "eslint-plugin-dagster-rules": "link:./rules",
     "eslint-plugin-import": "2.26.0",
     "eslint-plugin-jest": "^26.1.5",
     "eslint-plugin-jsx-a11y": "6.5.1",
@@ -23,7 +26,9 @@
     "eslint-plugin-react-hooks": "4.5.0"
   },
   "devDependencies": {
+    "@types/jest": "^27.5.1",
     "eslint": "^8.6.0",
-    "prettier": "2.2.1"
+    "prettier": "2.2.1",
+    "ts-jest": "^28.0.3"
   }
 }

js_modules/dagit/packages/eslint-config/rules/__tests__/missing-graphql-variables-type.test.js

@@ -0,0 +1,69 @@
+/* eslint-disable */
+const {ESLintUtils, AST_NODE_TYPES} = require('@typescript-eslint/utils');
+
+const ruleTester = new ESLintUtils.RuleTester({
+  parser: '@typescript-eslint/parser',
+});
+
+jest.mock('fs');
+// @ts-expect-error - using require because this package isn't setup for import declarations
+const fs = require('fs');
+
+// @ts-expect-error - using require because this package isn't setup for import declarations
+const {rule} = require('../missing-graphql-variables-type');
+
+fs.readFileSync = (path) => {
+  if (path.includes('WithVariables')) {
+    return `
+      export interface SomeQuery {}
+      export interface SomeQueryVariables {}
+    `;
+  } else {
+    return `
+      export interface SomeQuery {}
+    `;
+  }
+};
+
+ruleTester.run('missing-graphql-variables', rule, {
+  valid: [
+    `
+      import { SomeQuery, SomeQueryVariables } from '../SomeQueryWithVariables';
+      useQuery<SomeQuery, SomeQueryVariables>();
+    `,
+    `
+      import { SomeQuery } from '../SomeQueryWithOutVariables';
+      useQuery<SomeQuery>();
+    `,
+  ],
+  invalid: [
+    {
+      code: `
+        import { SomeQuery } from '../SomeQueryWithVariables';
+        useQuery<SomeQuery>();
+      `,
+      output: `
+        import { SomeQuery, SomeQueryVariables } from '../SomeQueryWithVariables';
+        useQuery<SomeQuery, SomeQueryVariables>();
+      `,
+      errors: [{
+        type: AST_NODE_TYPES.CallExpression, 
+        messageId: 'missing-graphql-variables-type', 
+      }],
+    },
+    {
+      code: `
+        import { SomeQuery, SomeQueryVariables } from '../SomeQueryWithVariables';
+        useQuery<SomeQuery>();
+      `,
+      output: `
+        import { SomeQuery, SomeQueryVariables } from '../SomeQueryWithVariables';
+        useQuery<SomeQuery, SomeQueryVariables>();
+      `,
+      errors: [{
+        type: AST_NODE_TYPES.CallExpression, 
+        messageId: 'missing-graphql-variables-type', 
+      }],
+    }
+  ],
+});

js_modules/dagit/packages/eslint-config/rules/index.js

@@ -0,0 +1,19 @@
+/* eslint-disable */
+const projectName = 'dagster-rules';
+
+const rules = {
+  'missing-graphql-variables-type': require('./missing-graphql-variables-type').rule,
+};
+
+module.exports = {
+  parser: `@typescript-eslint/parser`,
+  configs: {
+    all: {
+      plugins: [projectName],
+      rules: {
+        [`${projectName}/missing-graphql-variables-type`]: 'error',
+      },
+    },
+  },
+  rules,
+};

js_modules/dagit/packages/eslint-config/rules/missing-graphql-variables-type.js

@@ -0,0 +1,115 @@
+/* eslint-disable */
+
+const fs = require('fs');
+const path = require('path');
+const {ESLintUtils, AST_NODE_TYPES} = require('@typescript-eslint/utils');
+
+const createRule = ESLintUtils.RuleCreator((name) => name);
+
+/**
+ * Strategy:
+ *  1. Pass over all useQuery calls and append "Variables" to the identifierName of the first type
+ *    -- eg: useQuery<SomeQuery>() --> SomeQueryVariables
+ *  2. Check if "SomeQueryVariables" is exported from the file that "SomeQuery" is imported from.
+ *  3. If it's not then continue
+ *     If it is then check that the second type argument to useQuery is that SomeQueryVariables.
+ *     If not then throw an error.
+ *
+ */
+
+module.exports = {
+  rule: createRule({
+    create(context) {
+      return {
+        [AST_NODE_TYPES.CallExpression](node) {
+          const callee = node.callee;
+          if (callee.type !== 'Identifier') {
+            return;
+          }
+          // if it's not a useQuery call then ignore
+          if (callee.name !== 'useQuery') {
+            return;
+          }
+          const queryType =
+            node.typeParameters && node.typeParameters.params && node.typeParameters.params[0];
+          if (!queryType || queryType.type !== 'TSTypeReference') {
+            return;
+          }
+          if (queryType.typeName.type !== 'Identifier') {
+            return;
+          }
+          const queryName = queryType.typeName.name;
+          // if the type doesn't end with Query then ignore
+          if (!queryName.endsWith('Query')) {
+            return;
+          }
+          const variablesName = queryName + 'Variables';
+          let queryImportSpecifier = null;
+          const importDeclaration = context
+            .getSourceCode()
+            .ast.body.find(
+              (node) =>
+                node.type === 'ImportDeclaration' &&
+                node.specifiers.find(
+                  (node) => {
+                    if (node.type === 'ImportSpecifier' && node.local.name === queryName) {
+                      queryImportSpecifier = node;
+                      return true;
+                    }
+                  }
+                ),
+            )
+          const importPath = importDeclaration.source.value;
+          const currentPath = context.getFilename().split('/').slice(0, -1).join('/');
+          const fullPath = path.join(currentPath, importPath + '.ts');
+
+          const graphqlTypeFile = fs.readFileSync(fullPath, {encoding: 'utf8'});
+
+          // This part is kind of hacky. I should use the parser service to find the identifier
+          // but this is faster then tokenizing the whole file
+          if (!graphqlTypeFile.includes('export interface ' + variablesName)) {
+            return;
+          }
+          // This is a Query type with a generated QueryVariables type. Make sure we're using it
+          const secondType = node.typeParameters.params[1];
+          if (
+            !secondType ||
+            (secondType.type === 'TSTypeReference' &&
+              secondType.typeName.type === 'Identifier' &&
+              secondType.typeName.name !== variablesName)
+          ) {
+            context.report({
+              messageId: 'missing-graphql-variables-type',
+              node,
+              data: {
+                queryType: queryName,
+                variablesType: variablesName,
+              },
+              *fix(fixer) {
+                if (!importDeclaration.specifiers.find(node => node.type === 'ImportSpecifier' && node.local.name === variablesName)) {
+                  yield fixer.insertTextAfter(queryImportSpecifier, `, ${variablesName}`);
+                }
+                yield fixer.insertTextAfter(queryType, `, ${variablesName}`);
+              },
+            });
+          }
+        },
+      };
+    },
+    name: 'missing-graphql-variables-type',
+    meta: {
+      fixable: true,
+      docs: {
+        description: 'useQuery is missing QueryVariables parameter.',
+        recommended: 'error',
+      },
+      messages: {
+        'missing-graphql-variables-type':
+          '`useQuery<{{queryType}}>(...)` should be `useQuery<{{queryType}},{{variablesType}}>(...)`.',
+      },
+      type: 'problem',
+      schema: [],
+    },
+    defaultOptions: [],
+  }),
+};