Add .js file extension to all in-project imports (#10994)

* add `.js` file extension to all in-project imports

* bundler adjustments

* remove postprocessing step

* fixup, script to detect build differences

* more fixes

* changeset

* adjust jest file resolution

* fix up wrong import in `batchHttpLink`

* add AreTheTypesWrong check

* minor action changes

* fixup

* use ascii format for attw

* update AreTheTypesWrong, use new `--pack` option

* add build comparison action

* omit minified comparison output

* trigger ci

* eslint configuration changes, temp custom eslint version

* trigger ci

* give workflows distincive names?

* revert to latest working CI build

* revert changes to workflows

* clean up newly merged imports

* fix command name

* adjust fetch-depth

* use runner temp dir

* dir tweaks

* temp file dir?

* fir argument

* logging

* more detail logging

* fix binary logic

* remove a bunch of logging again

* disable `set`

* partially reintroduce

* now, to find that last command...

* this seems like a good compromise

* bump the TS eslint parser to deal with TS 5.1

* apply feedback from review

Author: phryneas

.attw.json

@@ -0,0 +1,6 @@
+{
+  "ignoreRules": [
+    "false-esm",
+    "cjs-resolves-to-esm"
+  ]
+}

.changeset/lemon-fans-shake.md

@@ -0,0 +1,5 @@
+---
+'@apollo/client': minor
+---
+
+Add .js file extensions to imports in src and dist/**/*.d.ts

.eslintrc

@@ -1,6 +1,6 @@
 {
   "parser": "@typescript-eslint/parser",
-  "plugins": ["@typescript-eslint"],
+  "plugins": ["@typescript-eslint", "import"],
   "env": {
     "browser": true,
     "node": true,
@@ -9,17 +9,35 @@
   "parserOptions": {
     "ecmaVersion": "latest"
   },
+  "settings": {
+    "import/parsers": {
+      "@typescript-eslint/parser": [".ts", ".tsx"]
+    },
+    "import/resolver": {
+      "typescript": {
+        "alwaysTryTypes": true
+      }
+    }
+  },
   "overrides": [
     {
       "files": ["**/*.ts", "**/*.tsx"],
       "excludedFiles": ["**/__tests__/**/*.*"],
       "rules": {
-        "@typescript-eslint/consistent-type-imports": ["error", { 
+        "@typescript-eslint/consistent-type-imports": ["error", {
           "prefer": "type-imports",
           "disallowTypeAnnotations": false,
           "fixStyle": "separate-type-imports"
         }],
-        "@typescript-eslint/no-import-type-side-effects": "error"
+        "@typescript-eslint/no-import-type-side-effects": "error",
+        "import/extensions": [
+          "error",
+          "always",
+          {
+            "ignorePackages": true,
+            "checkTypeImports": true
+          }
+        ]
       }
     },
     {
@@ -31,4 +49,7 @@
       }
     }
   ],
+  "rules": {
+    "import/no-unresolved": "error"
+  }
 }

.github/workflows/arethetypeswrong.yml

@@ -0,0 +1,28 @@
+name: AreTheTypesWrong
+on:
+  pull_request:
+    branches:
+      - main
+      - release-*
+
+concurrency: ${{ github.workflow }}-${{ github.ref }}
+
+jobs:
+  arethetypeswrong:
+    name: Are the types wrong
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repo
+        uses: actions/checkout@v3
+      - name: Setup Node.js 18.x
+        uses: actions/setup-node@v3
+        with:
+          node-version: 18.x
+      - name: Install dependencies (with cache)
+        uses: bahmutov/npm-install@v1
+
+      - name: Run build
+        run: npm run build
+      - name: Run AreTheTypesWrong
+        id: attw
+        run: ./node_modules/.bin/attw --format ascii --pack dist > $GITHUB_STEP_SUMMARY

.gitignore

@@ -70,3 +70,6 @@ junit.xml
 reports
 
 esbuild-why-*.html
+
+.yalc
+yalc.lock

config/entryPoints.js

@@ -88,6 +88,9 @@ exports.check = function (id, parentId) {
 function partsAfterDist(id) {
   const parts = id.split(path.sep);
   const distIndex = parts.lastIndexOf("dist");
+  if (/^index.jsx?$/.test(parts[parts.length - 1])) {
+    parts.pop();
+  }
   if (distIndex >= 0) {
     return parts.slice(distIndex + 1);
   }

config/jest.config.js

@@ -23,6 +23,7 @@ const defaults = {
       },
     ],
   },
+  resolver: "ts-jest-resolver",
 };
 
 const ignoreTSFiles = '.ts$';

config/postprocessDist.ts

@@ -1,7 +1,6 @@
-import * as fs from "fs";
-import * as path from "path";
-import resolve from "resolve";
-import { distDir, eachFile, reparse, reprint } from './helpers';
+import { distDir } from './helpers.ts';
+import fs from 'node:fs';
+import path from 'node:path';
 
 const globalTypesFile = path.resolve(distDir, "utilities/globals/global.d.ts");
 fs.writeFileSync(globalTypesFile, 
@@ -10,116 +9,4 @@ fs.writeFileSync(globalTypesFile,
     .filter(line => line.trim() !== 'const __DEV__: boolean;')
     .join("\n"), 
   "utf8"
-);
-
-// The primary goal of the 'npm run resolve' script is to make ECMAScript
-// modules exposed by Apollo Client easier to consume natively in web browsers,
-// without bundling, and without help from package.json files. It accomplishes
-// this goal by rewriting internal ./ and ../ (relative) imports to refer to a
-// specific ESM module (not a directory), including its file extension. Because
-// of this limited goal, this script only touches ESM modules that have .js file
-// extensions, not .cjs CommonJS bundles.
-
-// A secondary goal of this script is to enforce that any module using the
-// __DEV__ global constant imports the @apollo/client/utilities/globals polyfill
-// module first.
-
-eachFile(distDir, (file, relPath) => new Promise((resolve, reject) => {
-  fs.readFile(file, "utf8", (error, source) => {
-    if (error) return reject(error);
-
-    const tr = new Transformer;
-    const output = tr.transform(source, file);
-
-    if (source === output) {
-      resolve(file);
-    } else {
-      fs.writeFile(file, output, "utf8", error => {
-        error ? reject(error) : resolve(file);
-      });
-    }
-  });
-}));
-
-import * as recast from "recast";
-const n = recast.types.namedTypes;
-type Node = recast.types.namedTypes.Node;
-
-class Transformer {
-  absolutePaths = new Set<string>();
-
-  transform(code: string, file: string) {
-    const ast = reparse(code);
-    const transformer = this;
-
-    recast.visit(ast, {
-      visitImportDeclaration(path) {
-        this.traverse(path);
-        transformer.normalizeSourceString(file, path.node.source);
-      },
-
-      visitImportExpression(path) {
-        this.traverse(path);
-        transformer.normalizeSourceString(file, path.node.source);
-      },
-
-      visitExportAllDeclaration(path) {
-        this.traverse(path);
-        transformer.normalizeSourceString(file, path.node.source);
-      },
-
-      visitExportNamedDeclaration(path) {
-        this.traverse(path);
-        transformer.normalizeSourceString(file, path.node.source);
-      },
-    });
-
-    return reprint(ast);
-  }
-
-  isRelative(id: string) {
-    return id.startsWith("./") || id.startsWith("../");
-  }
-
-  normalizeSourceString(file: string, source?: Node | null) {
-    if (source && n.StringLiteral.check(source)) {
-      // We mostly only worry about normalizing _relative_ module identifiers,
-      // which start with a ./ or ../ and refer to other modules within the
-      // @apollo/client package, but we also manually normalize one non-relative
-      // identifier, ts-invariant/process, to prevent webpack 5 errors
-      // containing the phrase "failed to resolve only because it was resolved
-      // as fully specified," referring to webpack's resolve.fullySpecified
-      // option, which is apparently now true by default when the enclosing
-      // package's package.json file has "type": "module" (which became true for
-      // Apollo Client in v3.5).
-      if (source.value.split("/", 2).join("/") === "ts-invariant/process") {
-        source.value = "ts-invariant/process/index.js";
-      } else if (this.isRelative(source.value)) {
-        try {
-          source.value = this.normalizeId(source.value, file);
-        } catch (error) {
-          console.error(`Failed to resolve ${source.value} in ${file} with error ${error}`);
-          process.exit(1);
-        }
-      }
-    }
-  }
-
-  normalizeId(id: string, file: string) {
-    const basedir = path.dirname(file);
-    const absPath = resolve.sync(id, {
-      basedir,
-      extensions: [".mjs", ".js"],
-      packageFilter(pkg) {
-        return pkg.module ? {
-          ...pkg,
-          main: pkg.module,
-        } : pkg;
-      },
-    });
-    this.absolutePaths.add(absPath);
-    const relPath = path.relative(basedir, absPath);
-    const relId = relPath.split(path.sep).join('/');
-    return this.isRelative(relId) ? relId : "./" + relId;
-  }
-}
+);

config/processInvariants.ts

@@ -1,6 +1,6 @@
 import * as fs from 'fs';
 import { posix, join as osPathJoin } from 'path';
-import { distDir, eachFile, reparse, reprint } from './helpers';
+import { distDir, eachFile, reparse, reprint } from './helpers.ts';
 import type { ExpressionKind } from 'ast-types/lib/gen/kinds';
 
 eachFile(distDir, (file, relPath) => {
@@ -211,7 +211,6 @@ function transform(code: string, relativeFilePath: string) {
             b.literal(false)
           );
         }
-
         return node;
       },
     });

config/rewriteSourceMaps.ts

@@ -1,6 +1,6 @@
 import * as fs from "fs";
 import * as path from "path";
-import { distDir } from './helpers';
+import { distDir } from './helpers.ts';
 import glob = require("glob");
 
 glob(`${distDir.replace(/\\/g, '/')}/**/*.js.map`, (error, files) => {

config/rollup.config.js

@@ -1,4 +1,4 @@
-import path from 'path';
+import path, { resolve, dirname } from 'path';
 import { promises as fs } from "fs";
 
 import nodeResolve from '@rollup/plugin-node-resolve';
@@ -109,9 +109,8 @@ function prepareBundle({
 
   return {
     input: inputFile,
-    external(id, parentId) {
-      return isExternal(id, parentId, true);
-    },
+    // the external check is done by the `'externalize-dependency'` plugin
+    // external(id, parentId) {}
     output: {
       file: outputFile,
       format: 'cjs',
@@ -120,6 +119,29 @@ function prepareBundle({
       externalLiveBindings: false,
     },
     plugins: [
+      {
+        name: 'externalize-dependency',
+        resolveId(id, parentId) {
+          if (!parentId) {
+            return null;
+          }
+          function removeIndex(filename) {
+            if (filename.endsWith(`${path.sep}index.js`)) {
+              return filename.slice(0, -`${path.sep}index.js`.length)
+            }
+            return filename
+          }
+
+          const external = isExternal(id, parentId, true)
+          if (external) {
+            if (id.startsWith(".")) {
+              return { id: removeIndex(resolve(dirname(parentId), id)), external: true };
+            }
+            return { id: removeIndex(id), external: true };
+          }
+          return null;
+        }
+      },
       extensions ? nodeResolve({ extensions }) : nodeResolve(),
       {
         name: "copy *.cjs to *.cjs.native.js",

config/tsconfig.json

@@ -12,6 +12,8 @@
     "esModuleInterop": true,
     "sourceMap": true,
     "inlineSourceMap": false,
+    "noEmit": true,
+    "allowImportingTsExtensions": true,
     // This option should cause sourcesContent to be included in the
     // source map JSON, so we don't have to rely on the (nonexistent)
     // relative paths in the sources array, but it appears tsc does not