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.

Sign up for GitHub

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

Jump to bottom

chore: refactor tsconfigs to use project references #9038

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+859 −463
Draft

chore: refactor tsconfigs to use project references #9038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81d9f4f
chore: refactor tsconfigs to use project references
JamesHenry May 3, 2024
98923ad
chore: latest nx 19 stable
JamesHenry May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 0 additions & 14 deletions .yarn/patches/@nx-eslint-npm-17.3.1-a2f85d8c50.patch
View file
Open in desktop

This file was deleted.

11 changes: 2 additions & 9 deletions eslint.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,8 @@ export default tseslint.config(
project: [
'tsconfig.json',
'packages/*/tsconfig.json',
/**
* We are currently in the process of transitioning to nx's out of the box structure and
* so need to manually specify converted packages' tsconfig.build.json and tsconfig.spec.json
* files here for now in addition to the tsconfig.json glob pattern.
*
* TODO(#4665): Clean this up once all packages have been transitioned.
*/
'packages/scope-manager/tsconfig.build.json',
'packages/scope-manager/tsconfig.spec.json',
'packages/*/tsconfig.build.json',
'packages/*/tsconfig.spec.json',
],
tsconfigRootDir: __dirname,
warnOnUnsupportedTypeScriptVersion: false,
Expand Down
6 changes: 6 additions & 0 deletions nx.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
{
"$schema": "./node_modules/nx/schemas/nx-schema.json",
"nxCloudAccessToken": "YjIzMmMxMWItMjhiMS00NWY2LTk1NWYtYWU3YWQ0YjE4YjBlfHJlYWQ=",
"plugins": [
{
"plugin": "@nx/js/typescript",
"exclude": ["*", "packages/integration-tests/fixtures/**"]
}
],
"release": {
"projects": ["*"],
"changelog": {
Expand Down
12 changes: 6 additions & 6 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,10 @@
"@babel/types": "^7.24.0",
"@eslint/eslintrc": "^2.1.4",
"@eslint/js": "^8.57.0",
"@nx/eslint": "18.2.3",
"@nx/jest": "18.2.3",
"@nx/workspace": "18.2.3",
"@nx/eslint": "19.0.0",
"@nx/jest": "19.0.0",
"@nx/js": "19.0.0",
"@nx/workspace": "19.0.0",
"@swc/core": "^1.4.12",
"@swc/jest": "^0.2.36",
"@types/babel__code-frame": "^7.0.6",
Expand Down Expand Up @@ -112,7 +113,7 @@
"markdownlint-cli": "^0.39.0",
"ncp": "^2.0.0",
"netlify": "^13.1.14",
"nx": "18.2.3",
"nx": "19.0.0",
"prettier": "3.2.5",
"pretty-format": "^29.7.0",
"raw-loader": "^4.0.2",
Expand Down Expand Up @@ -141,8 +142,7 @@
"pretty-format": "^29",
"react-split-pane@^0.1.92": "patch:react-split-pane@npm%3A0.1.92#./.yarn/patches/react-split-pane-npm-0.1.92-93dbf51dff.patch",
"tsx": "^4.7.2",
"typescript": "5.4.3",
"@nx/eslint@17.3.1": "patch:@nx/eslint@npm%3A17.3.1#./.yarn/patches/@nx-eslint-npm-17.3.1-a2f85d8c50.patch"
"typescript": "5.4.3"
},
"packageManager": "yarn@3.8.1"
}
3 changes: 1 addition & 2 deletions packages/ast-spec/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@
"clean-fixtures": "rimraf -g \"./src/**/fixtures/**/snapshots\"",
"format": "prettier --write \"./**/*.{ts,mts,cts,tsx,js,mjs,cjs,jsx,json,md,css}\" --ignore-path ../../.prettierignore",
"lint": "npx nx lint",
"test": "jest",
"typecheck": "tsc -p tsconfig.json --noEmit"
"test": "jest"
},
"funding": {
"type": "opencollective",
Expand Down
1 change: 1 addition & 0 deletions packages/ast-spec/tests/BinaryOperatorToText.type-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type BinaryOperatorWithoutInvalidTypes = Exclude<
| SyntaxKind.CommaToken // -> SequenceExpression
| SyntaxKind.QuestionQuestionToken // -> LogicalExpression
>;

type _Test = {
readonly [T in BinaryOperatorWithoutInvalidTypes]: BinaryOperatorToText[T];
// If there are any BinaryOperator members that don't have a corresponding
Expand Down
2 changes: 1 addition & 1 deletion packages/ast-spec/tests/util/serializers/Node.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ const serializer: NewPlugin = {
const loc = node.loc;
const range = node.range;

const outputLines = [];
const outputLines: string[] = [];
const childIndentation = indentation + config.indent;

const printValue = (value: unknown): string =>
Expand Down
7 changes: 3 additions & 4 deletions packages/ast-spec/tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"composite": true,
"jsx": "preserve",
"outDir": "./dist",
"rootDir": "./src",
"rootDir": "src",
"outDir": "dist",
"emitDeclarationOnly": false,
Comment on lines +4 to +6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, hm, you're using emitDeclarationOnly, but only sometimes. It seems simpler to only set noEmit for test projects, then leave all emit on in the base config, then only have tsconfig.json and tsconfig.spec.json per-package if you want to split apart tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me know if my comment here #9038 (comment) alters your view on this please. Having type-checking not produce .js etc artifacts would seem to be desirable, and therefore we would need a way to distinguish between type-checking vs building

"resolveJsonModule": true
},
"include": ["src", "typings"],
Expand Down
17 changes: 11 additions & 6 deletions packages/ast-spec/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
{
"extends": "./tsconfig.build.json",
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"composite": false,
"rootDir": "."
"outDir": "../../dist/out-tsc/ast-spec"
Comment on lines -2 to +4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a "solution" file, there's actually no reason to extend anything here; these options are all noops.

But, I'm not sure that this layout is quite right, depending on other tooling which may assume that the tsconfig contains the options for the code. Maybe this extension is why you dont' have problems?

},
"include": ["src", "typings", "tests", "tools", "**/fixtures/**/config.ts"],
"exclude": ["**/fixtures/**/fixture.ts", "**/fixtures/**/fixture.tsx"],
"references": [{ "path": "../typescript-estree/tsconfig.build.json" }]
"files": [],
"references": [
{
"path": "./tsconfig.build.json"
},
{
"path": "./tsconfig.spec.json"
}
]
Comment on lines +6 to +14
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is interesting; I assume these are the main code and the tests, in which case I would have actually expected you to have tsconfig.json here be the main code, then have a tsconfig.test.json (or spec, that's bikeshedding) separate and then referenced from the repo root

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind this approach/config structure was as follows:

  • A tsconfig.json for a project will power its type-checking. This is is not expected to produce any .js etc artifacts. It is logically expected to type-check all files, both production and test files.
  • An optional tsconfig.lib.json (customizable name, in this workspace the convention is tsconfig.build.json) would be used to control a tsc powered build, in which .js etc artifacts are produced. It should naturally not include any test files as part of its build process.
    • The presence of this file is also how Nx, via the new plugin, can infer that a build "target" should exist for this project (meaning you can run nx build ast-spec and it just works, including distributable caching and other core Nx features, without any specific Nx config at the project level.
  • An optional tsconfig.spec.json seemed to make sense as a corollary of the tsconfig.lib.json/tsconfig.build.json, but note that its presence is not used to infer any targets. It is only there to pick up all the files that the build config would exclude and possible be passed to a test runner like jest with ts-jest as config.

Does that all make sense? Is there a different/more optimal way to achieve the above objectives around having typecheck and build be separate actions/targets?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That does make some sense, I was jus tlooking for ways to simplify things. But this is not dissimilar to how vite does things where it makes an extra tsconfig that's referenced just for config files.

}
30 changes: 30 additions & 0 deletions packages/ast-spec/tsconfig.spec.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
"outDir": "../../dist/out-tsc/ast-spec",
"types": ["jest", "node"],
"noImplicitAny": false
},
"exclude": ["tests/fixtures"],
"include": [
"tests",
"tools",
"**/*.test.ts",
"**/*.spec.ts",
"**/*.test.tsx",
"**/*.spec.tsx",
"**/*.test.js",
"**/*.spec.js",
"**/*.test.jsx",
"**/*.spec.jsx",
"**/*.d.ts"
],
"references": [
{
"path": "../typescript-estree/tsconfig.build.json"
},
{
"path": "./tsconfig.build.json"
}
]
}
7 changes: 3 additions & 4 deletions packages/eslint-plugin-internal/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,12 @@
"main": "dist/index.js",
"types": "index.d.ts",
"scripts": {
"build": "tsc -b tsconfig.build.json",
"clean": "tsc -b tsconfig.build.json --clean",
"build": "npx tsc -b tsconfig.build.json",
"clean": "npx tsc -b tsconfig.build.json --clean",
"postclean": "rimraf dist && rimraf coverage",
"format": "prettier --write \"./**/*.{ts,mts,cts,tsx,js,mjs,cjs,jsx,json,md,css}\" --ignore-path ../../.prettierignore",
"lint": "npx nx lint",
"test": "jest --coverage",
"typecheck": "tsc -p tsconfig.json --noEmit"
"test": "jest --coverage"
},
"dependencies": {
"@prettier/sync": "^0.5.1",
Expand Down
20 changes: 16 additions & 4 deletions packages/eslint-plugin-internal/tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,25 @@
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
// specifically disable declarations for the plugin
// specifically disable declarations for the plugin (which requires disabling composite)
"composite": false,
"declaration": false,
"declarationMap": false,
"outDir": "./dist",
"rootDir": "./src",
"emitDeclarationOnly": false,
"rootDir": "src",
"outDir": "dist",
"resolveJsonModule": true
},
"include": ["src", "typings"],
"references": [{ "path": "../utils/tsconfig.build.json" }]
"references": [
{
"path": "../rule-tester/tsconfig.build.json"
},
{
"path": "../utils/tsconfig.build.json"
},
{
"path": "../type-utils/tsconfig.build.json"
}
]
}
20 changes: 17 additions & 3 deletions packages/eslint-plugin-internal/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,24 @@
{
"extends": "./tsconfig.build.json",
"extends": "../../tsconfig.base.json",
"compilerOptions": {
// specifically disable declarations for the plugin (which requires disabling composite)
"composite": false,
"target": "ES2022",
"emitDeclarationOnly": false,
"noEmit": true,
"declaration": false,
"declarationMap": false,
"rootDir": "."
},
"include": ["src", "typings", "tests", "index.d.ts"],
"references": [{ "path": "../utils/tsconfig.build.json" }]
"references": [
{
"path": "../rule-tester/tsconfig.build.json"
},
{
"path": "../utils/tsconfig.build.json"
},
{
"path": "../type-utils/tsconfig.build.json"
}
]
}
3 changes: 1 addition & 2 deletions packages/eslint-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@
"generate:configs": "npx nx run repo-tools:generate-configs",
"lint": "npx nx lint",
"test": "cross-env NODE_OPTIONS=\"--experimental-vm-modules\" jest --coverage --logHeapUsage",
"test-single": "cross-env NODE_OPTIONS=\"--experimental-vm-modules\" jest --no-coverage",
"typecheck": "tsc -p tsconfig.json --noEmit"
"test-single": "cross-env NODE_OPTIONS=\"--experimental-vm-modules\" jest --no-coverage"
},
"dependencies": {
"@eslint-community/regexpp": "^4.10.0",
Expand Down
14 changes: 8 additions & 6 deletions packages/eslint-plugin/src/rules/naming-convention-utils/enums.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,17 +134,19 @@ enum TypeModifiers {
type TypeModifiersString = keyof typeof TypeModifiers;

export {
IndividualAndMetaSelectorsString,
MetaSelectors,
MetaSelectorsString,
Modifiers,
ModifiersString,
PredefinedFormats,
PredefinedFormatsString,
Selectors,
SelectorsString,
TypeModifiers,
TypeModifiersString,
UnderscoreOptions,
};
export type {
IndividualAndMetaSelectorsString,
MetaSelectorsString,
ModifiersString,
PredefinedFormatsString,
SelectorsString,
TypeModifiersString,
UnderscoreOptionsString,
};
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/rules/naming-convention.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,4 +784,4 @@ function requiresQuoting(
return _requiresQuoting(name, target);
}

export { MessageIds, Options };
export type { MessageIds, Options };
3 changes: 1 addition & 2 deletions packages/eslint-plugin/src/util/explicitReturnTypeUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,8 @@ export {
checkFunctionExpressionReturnType,
checkFunctionReturnType,
doesImmediatelyReturnFunctionExpression,
FunctionExpression,
FunctionNode,
isTypedFunctionExpression,
isValidFunctionExpressionReturnType,
ancestorHasReturnType,
};
export type { FunctionExpression, FunctionNode };
3 changes: 1 addition & 2 deletions packages/eslint-plugin/src/util/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export {
isObjectNotArray,
getParserServices,
nullThrows,
InferMessageIdsTypeFromRule,
InferOptionsTypeFromRule,
NullThrowsReasons,
};
export type { InferMessageIdsTypeFromRule, InferOptionsTypeFromRule };
4 changes: 1 addition & 3 deletions packages/eslint-plugin/src/util/misc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,6 @@ function isParenlessArrowFunction(
export {
arrayGroupByToMap,
arraysAreEqual,
Equal,
ExcludeKeys,
findFirstResult,
formatWordList,
getEnumNames,
Expand All @@ -244,8 +242,8 @@ export {
isRestParameterDeclaration,
isParenlessArrowFunction,
MemberNameType,
RequireKeys,
typeNodeRequiresParentheses,
upperCaseFirst,
findLastIndex,
};
export type { Equal, ExcludeKeys, RequireKeys };
27 changes: 20 additions & 7 deletions packages/eslint-plugin/tsconfig.build.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,32 @@
{
"extends": "../../tsconfig.base.json",
"compilerOptions": {
// specifically disable declarations for the plugin
// specifically disable declarations for the plugin (which requires disabling composite)
// see reasoning in packages/eslint-plugin/rules.d.ts
"composite": false,
"declaration": false,
"declarationMap": false,
"outDir": "./dist",
"rootDir": "./src",
"emitDeclarationOnly": false,
"rootDir": "src",
"outDir": "dist",
"resolveJsonModule": true
},
"include": ["src", "typings"],
"references": [
{ "path": "../utils/tsconfig.build.json" },
{ "path": "../parser/tsconfig.build.json" },
{ "path": "../scope-manager/tsconfig.build.json" },
{ "path": "../type-utils/tsconfig.build.json" }
{
"path": "../utils/tsconfig.build.json"
},
{
"path": "../parser/tsconfig.build.json"
},
{
"path": "../rule-tester/tsconfig.build.json"
},
{
"path": "../scope-manager/tsconfig.build.json"
},
{
"path": "../type-utils/tsconfig.build.json"
}
]
}