Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 41 commits into from
Jul 7, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
324c59e
add `.js` file extension to all in-project imports
phryneas Jun 20, 2023
3173f83
bundler adjustments
phryneas Jun 22, 2023
fdd53e6
remove postprocessing step
phryneas Jun 23, 2023
7071b28
fixup, script to detect build differences
phryneas Jun 23, 2023
105764b
more fixes
phryneas Jun 23, 2023
78f8f23
changeset
phryneas Jun 23, 2023
d64a965
adjust jest file resolution
phryneas Jun 23, 2023
8708fa3
fix up wrong import in `batchHttpLink`
phryneas Jun 23, 2023
da2131f
add AreTheTypesWrong check
phryneas Jun 26, 2023
933b91f
minor action changes
phryneas Jun 26, 2023
05297ce
Merge remote-tracking branch 'origin/release-3.8' into pr/fileExtensions
phryneas Jun 26, 2023
07d3410
fixup
phryneas Jun 26, 2023
6589871
use ascii format for attw
phryneas Jun 26, 2023
192f64c
update AreTheTypesWrong, use new `--pack` option
phryneas Jun 30, 2023
fdd4dce
add build comparison action
phryneas Jun 30, 2023
270cc91
omit minified comparison output
phryneas Jun 30, 2023
9353904
trigger ci
phryneas Jun 30, 2023
2c210c7
eslint configuration changes, temp custom eslint version
phryneas Jul 5, 2023
3f62df1
trigger ci
phryneas Jul 6, 2023
957e48b
give workflows distincive names?
phryneas Jul 6, 2023
45249c4
revert to latest working CI build
phryneas Jul 6, 2023
f02f584
Merge remote-tracking branch 'origin/release-3.8' into pr/fileExtensions
phryneas Jul 6, 2023
0cc6868
revert changes to workflows
phryneas Jul 6, 2023
0640eaf
clean up newly merged imports
phryneas Jul 6, 2023
d109fbe
fix command name
phryneas Jul 6, 2023
c788a07
adjust fetch-depth
phryneas Jul 6, 2023
7b79d6f
use runner temp dir
phryneas Jul 6, 2023
482f31c
dir tweaks
phryneas Jul 6, 2023
41ff620
temp file dir?
phryneas Jul 6, 2023
446f00a
fir argument
phryneas Jul 6, 2023
5ce25a8
logging
phryneas Jul 6, 2023
0458e17
more detail logging
phryneas Jul 6, 2023
9de8acc
fix binary logic
phryneas Jul 6, 2023
3bf4092
remove a bunch of logging again
phryneas Jul 6, 2023
d46c65c
disable `set`
phryneas Jul 6, 2023
b90aa50
partially reintroduce
phryneas Jul 6, 2023
86c3b76
now, to find that last command...
phryneas Jul 6, 2023
e782d42
this seems like a good compromise
phryneas Jul 6, 2023
a1c5497
Merge remote-tracking branch 'origin/release-3.8' into pr/fileExtensions
phryneas Jul 7, 2023
51e0d17
bump the TS eslint parser to deal with TS 5.1
phryneas Jul 7, 2023
a53a948
apply feedback from review
phryneas Jul 7, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .attw.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"ignoreRules": [
"false-esm",
"cjs-resolves-to-esm"
]
}
5 changes: 5 additions & 0 deletions .changeset/lemon-fans-shake.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/client': minor
---

Add .js file extensions to imports in src and dist/**/*.d.ts
7 changes: 4 additions & 3 deletions .eslintrc
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"parser": "@typescript-eslint/parser",
"plugins": ["@typescript-eslint"],
"plugins": ["@typescript-eslint", "import"],
"env": {
"browser": true,
"node": true,
Expand All @@ -14,12 +14,13 @@
"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 } ]
}
},
{
Expand Down
29 changes: 29 additions & 0 deletions .github/workflows/arethetypeswrong.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
name: AreTheTypesWrong
on:
pull_request:
branches:
- main
- release-*

concurrency: ${{ github.workflow }}-${{ github.ref }}

jobs:
build:
name: Build library
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 && cd dist; npm pack

- name: Run AreTheTypesWrong
id: attw
run: ./node_modules/.bin/attw ./dist/*.tgz --format ascii > $GITHUB_STEP_SUMMARY
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,6 @@ junit.xml
reports

esbuild-why-*.html

.yalc
yalc.lock
26 changes: 26 additions & 0 deletions config/compare-build-output-to.sh
phryneas marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
upstream=$1
comparison=/tmp/comparison_checkout
root=$(git rev-parse --show-toplevel)


patterndiff(){
cd dist
find -name "$1" -exec bash -c "echo {}; diff <(cat '$comparison/dist/'{} | tr \"'\" '\"') <(cat {} | tr \"'\" '\"' ) -w" \; > ../diff."$1".diff
cd ..
}

[ -z "$upstream" ] && { echo "need upstream argument"; exit 1; }

git worktree add --force --detach --checkout "$comparison" "$upstream"

cd "$comparison"
yarn
yarn build
cd "$root"
yarn build

patterndiff "*.js"
patterndiff "*.cjs"
patterndiff "*.d.ts"

diff -r "$comparison/dist" "dist" -x "*.map" -x "*.native.*" -x "*.js" -x "*.cjs" -x "*.d.ts" -w > diff.rest.diff
3 changes: 3 additions & 0 deletions config/entryPoints.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions config/jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const defaults = {
},
],
},
resolver: "ts-jest-resolver",
};

const ignoreTSFiles = '.ts$';
Expand Down
128 changes: 9 additions & 119 deletions config/postprocessDist.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
import * as fs from "fs";
import * as path from "path";
import resolve from "resolve";
import { distDir, eachFile, reparse, reprint } from './helpers';
import { distDir, eachFile } from './helpers';

// 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,
Expand All @@ -16,123 +13,16 @@ import { distDir, eachFile, reparse, reprint } from './helpers';
// module first.

eachFile(distDir, (file, relPath) => new Promise((resolve, reject) => {
fs.readFile(file, "utf8", (error, source) => {
if (error) return reject(error);
resolve(file);

const tr = new Transformer;
const output = tr.transform(source, file);
// all the transformer did here was to add `.js` or `/index.js` to the end
// of relative imports (and the one external import 'ts-invariant/process')
// and track those imports for the `__DEV__` check below

if (
/\b__DEV__\b/.test(source) &&
// Ignore modules that reside within @apollo/client/utilities/globals.
relPath.split(path.sep, 2).join("/") !== "utilities/globals"
) {
let importsUtilitiesGlobals = false;

tr.absolutePaths.forEach(absPath => {
const distRelativePath =
path.relative(distDir, absPath).split(path.sep).join("/");
if (distRelativePath === "utilities/globals/index.js") {
importsUtilitiesGlobals = true;
}
});

if (!importsUtilitiesGlobals) {
reject(new Error(`Module ${
relPath
} uses __DEV__ but does not import @apollo/client/utilities/globals`));
}
}

if (source === output) {
resolve(file);
} else {
fs.writeFile(file, output, "utf8", error => {
error ? reject(error) : resolve(file);
});
}
});
// since we have all the file extensions in the src directory now, we can
// skip that part
// and the other PR will remove the __DEV__ check, so this whole file can
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

// soon be removed
}));

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;
}
}
4 changes: 2 additions & 2 deletions config/processInvariants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ function transform(code: string, relativeFilePath: string) {
const normalized = posix.normalize(
posix.join(posix.dirname(relativeFilePath), importedModuleId)
);
if (normalized === 'utilities/globals') {
if (normalized === 'utilities/globals/index.js') {
foundExistingImportDecl = true;
if (
node.specifiers?.some((s) =>
Expand All @@ -242,7 +242,7 @@ function transform(code: string, relativeFilePath: string) {
throw new Error(
`Missing import from "${posix.relative(
posix.dirname(relativeFilePath),
'utilities/globals'
'utilities/globals/index.js'
)} in ${relativeFilePath}`
);
}
Expand Down
31 changes: 29 additions & 2 deletions config/rollup.config.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -109,9 +109,12 @@ function prepareBundle({

return {
input: inputFile,
/*
external(id, parentId) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed anymore? I see its commented out, so perhaps we can just remove it altogether?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's quite a bunch of stuff that can be completely removed at this point - also the full postprocessDist.ts - I'm gonna do another cleanup on this :)

console.log({id, parentId})
return isExternal(id, parentId, true);
},
*/
output: {
file: outputFile,
format: 'cjs',
Expand All @@ -120,6 +123,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",
Expand All @@ -137,7 +163,8 @@ function prepareBundle({

export default [
...entryPoints.map(prepareBundle),
// Convert the ESM entry point to a single CJS bundle.
//.filter(x => x.input.includes("utilities")),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//.filter(x => x.input.includes("utilities")),

Do we need this line?

// Convert the ESM entry point to a single CJS bundle.#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Convert the ESM entry point to a single CJS bundle.#
// Convert the ESM entry point to a single CJS bundle.

prepareCJS(
'./dist/index.js',
'./dist/apollo-client.cjs',
Expand Down
Loading