Merge pull request #1092 from spotify/rugvip/verify-types

scripts: add check-type-dependencies to make sure @type dependencies are listed correctly

.github/workflows/frontend.yml

@@ -71,6 +71,9 @@ jobs:
         if: ${{ steps.yarn-lock.outcome == 'failure' }}
         run: yarn lerna -- run build
 
+      - name: verify type dependencies
+        run: yarn lint:type-deps
+
       - name: test changed packages
         if: ${{ steps.yarn-lock.outcome == 'success' }}
         run: yarn lerna -- run test --since origin/master -- --coverage

.github/workflows/master.yml

@@ -60,6 +60,9 @@ jobs:
       - name: build
         run: yarn build
 
+      - name: verify type dependencies
+        run: yarn lint:type-deps
+
       - name: test
         run: yarn lerna -- run test -- --coverage
 

docs/getting-started/development-environment.md

@@ -44,6 +44,7 @@ yarn build # Build published versions of packages, depends on tsc
 
 yarn lint # lint packages that have changed since later commit on origin/master
 yarn lint:all # lint all packages
+yarn lint:type-deps # verify that @types/* dependencies are placed correctly in packages
 
 yarn test # test packages that have changed since later commit on origin/master
 yarn test:all # test all packages

package.json

@@ -15,6 +15,7 @@
     "test:all": "lerna run test -- --coverage",
     "lint": "lerna run lint --since origin/master --",
     "lint:all": "lerna run lint --",
+    "lint:type-deps": "node scripts/check-type-dependencies.js",
     "docker-build": "yarn bundle && docker build . -t spotify/backstage",
     "create-plugin": "backstage-cli create-plugin",
     "remove-plugin": "backstage-cli remove-plugin",

packages/core-api/package.json

@@ -31,9 +31,7 @@
     "@backstage/theme": "^0.1.1-alpha.6",
     "@material-ui/core": "^4.9.1",
     "@material-ui/icons": "^4.9.1",
-    "@types/jest": "^25.2.2",
-    "@types/node": "^12.0.0",
-    "@types/zen-observable": "^0.8.0",
+    "@types/react": "^16.9",
     "prop-types": "^15.7.2",
     "react": "^16.12.0",
     "react-router-dom": "^5.2.0",
@@ -46,6 +44,9 @@
     "@testing-library/jest-dom": "^5.7.0",
     "@testing-library/react": "^9.3.2",
     "@testing-library/user-event": "^10.2.4",
+    "@types/jest": "^25.2.2",
+    "@types/node": "^12.0.0",
+    "@types/zen-observable": "^0.8.0",
     "jest-fetch-mock": "^3.0.3"
   },
   "files": [

packages/core-api/src/routing/index.ts

@@ -16,3 +16,4 @@
 
 export * from './types';
 export { createRouteRef } from './RouteRef';
+export type { MutableRouteRef } from './RouteRef';

packages/core/package.json

@@ -33,13 +33,8 @@
     "@material-ui/core": "^4.9.1",
     "@material-ui/icons": "^4.9.1",
     "@material-ui/lab": "4.0.0-alpha.45",
-    "@types/classnames": "^2.2.9",
-    "@types/google-protobuf": "^3.7.2",
-    "@types/jest": "^25.2.2",
-    "@types/node": "^12.0.0",
-    "@types/react-helmet": "^5.0.15",
+    "@types/react": "^16.9",
     "@types/react-sparklines": "^1.7.0",
-    "@types/zen-observable": "^0.8.0",
     "classnames": "^2.2.6",
     "clsx": "^1.1.0",
     "lodash": "^4.17.15",
@@ -61,6 +56,12 @@
     "@testing-library/jest-dom": "^5.7.0",
     "@testing-library/react": "^9.3.2",
     "@testing-library/user-event": "^10.2.4",
+    "@types/classnames": "^2.2.9",
+    "@types/google-protobuf": "^3.7.2",
+    "@types/jest": "^25.2.2",
+    "@types/node": "^12.0.0",
+    "@types/react-helmet": "^5.0.15",
+    "@types/zen-observable": "^0.8.0",
     "jest-fetch-mock": "^3.0.3"
   },
   "files": [

packages/dev-utils/package.json

@@ -37,14 +37,17 @@
     "@testing-library/jest-dom": "^5.7.0",
     "@testing-library/react": "^9.3.2",
     "@testing-library/user-event": "^10.2.4",
-    "@types/jest": "^25.2.2",
-    "@types/node": "^12.0.0",
+    "@types/react": "^16.9",
     "react": "^16.12.0",
     "react-dom": "^16.12.0",
     "react-hot-loader": "^4.12.21",
     "react-router": "^5.2.0",
     "react-router-dom": "^5.2.0"
   },
+  "devDependencies": {
+    "@types/jest": "^25.2.2",
+    "@types/node": "^12.0.0"
+  },
   "files": [
     "dist/**/*.{js,d.ts}"
   ]

packages/test-utils-core/package.json

@@ -30,11 +30,14 @@
   "dependencies": {
     "@testing-library/jest-dom": "^5.7.0",
     "@testing-library/react": "^9.3.2",
-    "@types/jest": "^25.2.2",
-    "@types/node": "^12.0.0",
+    "@types/react": "^16.9",
     "react": "^16.12.0",
     "react-dom": "^16.12.0"
   },
+  "devDependencies": {
+    "@types/jest": "^25.2.2",
+    "@types/node": "^12.0.0"
+  },
   "files": [
     "dist/**/*.{js,d.ts}"
   ]

packages/test-utils/package.json

@@ -36,13 +36,16 @@
     "@testing-library/jest-dom": "^5.7.0",
     "@testing-library/react": "^9.3.2",
     "@testing-library/user-event": "^10.2.4",
-    "@types/jest": "^25.2.2",
-    "@types/node": "^12.0.0",
+    "@types/react": "^16.9",
     "react": "^16.12.0",
     "react-dom": "^16.12.0",
     "react-router": "^5.2.0",
     "react-router-dom": "^5.2.0"
   },
+  "devDependencies": {
+    "@types/jest": "^25.2.2",
+    "@types/node": "^12.0.0"
+  },
   "files": [
     "dist/**/*.{js,d.ts}"
   ]

plugins/tech-radar/package.json

@@ -26,6 +26,7 @@
     "@material-ui/core": "^4.9.1",
     "@material-ui/icons": "^4.9.1",
     "@material-ui/lab": "4.0.0-alpha.45",
+    "@types/react": "^16.9",
     "color": "^3.1.2",
     "d3-force": "^2.0.1",
     "prop-types": "^15.7.2",

scripts/.eslintrc.js

@@ -0,0 +1,6 @@
+module.exports = {
+  extends: [require.resolve('@backstage/cli/config/eslint')],
+  rules: {
+    'no-console': 0,
+  },
+};

scripts/check-type-dependencies.js

@@ -0,0 +1,194 @@
+#!/usr/bin/env node
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+const fs = require('fs');
+const { resolve: resolvePath } = require('path');
+// Cba polluting root package.json, we'll have this
+// eslint-disable-next-line import/no-extraneous-dependencies
+const chalk = require('chalk');
+
+async function main() {
+  // This is from lerna, and cba polluting root package.json
+  // eslint-disable-next-line import/no-extraneous-dependencies
+  const LernaProject = require('@lerna/project');
+  const project = new LernaProject(resolvePath('.'));
+  const packages = await project.getPackages();
+
+  let hadErrors = false;
+
+  for (const pkg of packages) {
+    if (!shouldCheckTypes(pkg)) {
+      continue;
+    }
+    const { errors } = await checkTypes(pkg);
+    if (errors.length) {
+      hadErrors = true;
+      console.error(
+        `Incorrect type dependencies in ${chalk.yellow(pkg.name)}:`,
+      );
+      for (const error of errors) {
+        if (error.name === 'WrongDepError') {
+          console.error(
+            `  Move from ${chalk.red(error.from)} to ${chalk.green(
+              error.to,
+            )}: ${chalk.cyan(error.dep)}`,
+          );
+        } else if (error.name === 'MissingDepError') {
+          console.error(
+            `  Missing a type dependency: ${chalk.cyan(error.dep)}`,
+          );
+        } else {
+          console.error(`  Unknown error, ${chalk.red(error)}`);
+        }
+      }
+    }
+  }
+
+  if (hadErrors) {
+    console.error();
+    console.error(
+      chalk.red('At least one package had incorrect type dependencies'),
+    );
+
+    process.exit(2);
+  }
+}
+
+function shouldCheckTypes(pkg) {
+  return !pkg.private && pkg.get('types');
+}
+
+/**
+ * Scan index.d.ts for imports and return errors for any dependency that's
+ * missing or incorrect in package.json
+ */
+function checkTypes(pkg) {
+  const typeDecl = fs.readFileSync(
+    resolvePath(pkg.location, 'dist/index.d.ts'),
+    'utf8',
+  );
+  const deps = (typeDecl.match(/from '.*'/g) || [])
+    .map(match => match.replace(/from '(.*)'/, '$1'))
+    .filter(n => !n.startsWith('.'));
+
+  const errors = [];
+  const typeDeps = [];
+  for (const dep of deps) {
+    try {
+      const typeDep = findTypesPackage(dep, pkg);
+      if (typeDep) {
+        typeDeps.push(typeDep);
+      }
+    } catch (error) {
+      errors.push(error);
+    }
+  }
+
+  errors.push(...findTypeDepErrors(typeDeps, pkg));
+
+  return { errors };
+}
+
+/**
+ * Find the package used for types. This assumes that types are working is a package
+ * can be resolved, it doesn't do any checking of presence of types inside the dep.
+ */
+function findTypesPackage(dep, pkg) {
+  try {
+    require.resolve(`@types/${dep}/package.json`, { paths: [pkg.location] });
+    return `@types/${dep}`;
+  } catch {
+    try {
+      require.resolve(dep, { paths: [pkg.location] });
+      return undefined;
+    } catch {
+      try {
+        // Some type-only modules don't have a working main field, so try resolving package.json too
+        require.resolve(`${dep}/package.json`, { paths: [pkg.location] });
+        return undefined;
+      } catch {
+        try {
+          // Finally check if it's just a .d.ts file
+          require.resolve(`${dep}.d.ts`, { paths: [pkg.location] });
+          return undefined;
+        } catch {
+          throw mkErr('MissingDepError', `No types for ${dep}`, { dep });
+        }
+      }
+    }
+  }
+}
+
+/**
+ * Figures out what type dependencies are missing, or should be moved between dep types
+ */
+function findTypeDepErrors(typeDeps, pkg) {
+  const devDeps = mkTypeDepSet(pkg.get('devDependencies'));
+  const deps = mkTypeDepSet(pkg.get('dependencies'));
+
+  const errors = [];
+  for (const typeDep of typeDeps) {
+    if (!deps.has(typeDep)) {
+      if (devDeps.has(typeDep)) {
+        errors.push(
+          mkErr('WrongDepError', `Should be dep ${typeDep}`, {
+            dep: typeDep,
+            from: 'devDependencies',
+            to: 'dependencies',
+          }),
+        );
+      } else {
+        errors.push(
+          mkErr('MissingDepError', `No types for ${typeDep}`, {
+            dep: typeDep,
+          }),
+        );
+      }
+    } else {
+      deps.delete(typeDep);
+    }
+  }
+
+  for (const dep of deps) {
+    errors.push(
+      mkErr('WrongDepError', `Should be dev dep ${dep}`, {
+        dep,
+        from: 'dependencies',
+        to: 'devDependencies',
+      }),
+    );
+  }
+
+  return errors;
+}
+
+function mkTypeDepSet(deps) {
+  const typeDeps = Object.keys(deps || {}).filter(n => n.startsWith('@types/'));
+  return new Set(typeDeps);
+}
+
+function mkErr(name, msg, extra) {
+  const error = new Error(msg);
+  error.name = name;
+  Object.assign(error, extra);
+  return error;
+}
+
+main().catch(error => {
+  console.error(error.stack || error);
+  process.exit(1);
+});

yarn.lock

@@ -4417,7 +4417,7 @@
   dependencies:
     "@types/react" "*"
 
-"@types/react@*", "@types/react@^16.8.19":
+"@types/react@*", "@types/react@^16.8.19", "@types/react@^16.9":
   version "16.9.25"
   resolved "https://registry.npmjs.org/@types/react/-/react-16.9.25.tgz#6ae2159b40138c792058a23c3c04fd3db49e929e"
   integrity sha512-Dlj2V72cfYLPNscIG3/SMUOzhzj7GK3bpSrfefwt2YT9GLynvLCCZjbhyF6VsT0q0+aRACRX03TDJGb7cA0cqg==