chore: refactor tsconfigs to use project references #9038

JamesHenry · 2024-05-02T12:28:02Z

No description provided.

typescript-eslint · 2024-05-02T12:28:06Z

Thanks for the PR, @JamesHenry!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

netlify · 2024-05-02T12:28:19Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`98923ad`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/663a260acf47890009796d4e
😎 Deploy Preview	https://deploy-preview-9038--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 3 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-05-02T12:30:08Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 98923ad. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

codecov · 2024-05-02T12:52:32Z

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 88.09%. Comparing base (7984ef7) to head (98923ad).
Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9038      +/-   ##
==========================================
+ Coverage   88.04%   88.09%   +0.04%     
==========================================
  Files         410      409       -1     
  Lines       14186    14175      -11     
  Branches     4135     4135              
==========================================
- Hits        12490    12487       -3     
+ Misses       1391     1383       -8     
  Partials      305      305

Flag	Coverage Δ
unittest	`88.09% <88.23%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...-plugin/src/rules/naming-convention-utils/enums.ts	`100.00% <ø> (ø)`
...kages/eslint-plugin/src/rules/naming-convention.ts	`94.50% <ø> (ø)`
.../eslint-plugin/src/util/explicitReturnTypeUtils.ts	`97.61% <ø> (ø)`
packages/eslint-plugin/src/util/misc.ts	`98.63% <ø> (ø)`
packages/scope-manager/src/analyze.ts	`61.11% <100.00%> (ø)`
packages/scope-manager/src/index.ts	`100.00% <100.00%> (+27.27%)`	⬆️
...kages/scope-manager/src/referencer/ClassVisitor.ts	`78.78% <ø> (ø)`
...ges/scope-manager/src/referencer/PatternVisitor.ts	`68.29% <100.00%> (ø)`
packages/scope-manager/src/referencer/Reference.ts	`90.32% <100.00%> (ø)`
...ackages/scope-manager/src/referencer/Referencer.ts	`95.52% <100.00%> (ø)`
... and 13 more

... and 4 files with indirect coverage changes

bradzacher · 2024-05-02T22:46:25Z

tsconfig.base.json

    "noImplicitReturns": true,
-    "paths": {},
+    "noUnusedLocals": true,


can we turn this off?
no-unused-vars does this without causing TS errors (so we can remove the @ts-expect-errors)
it also means the build won't error out due to a measly unused variable.

Cool yeah I just took a relatively strong starting point inspired by other projects, we can change any of it to whatever's best. It isn't something that is generated or controlled by Nx in any way, it's completely up to us

bradzacher · 2024-05-02T22:47:16Z

packages/visitor-keys/src/index.ts

+export { visitorKeys } from './visitor-keys';
+export type { VisitorKeys } from './visitor-keys';


we should probably turn on our consistent-type-exports rule

Yeah all of these type changes allowed compilation isolatedModules to work

bradzacher · 2024-05-02T22:48:38Z

tsconfig.base.json

+    "lib": ["es2022"],
+    "module": "NodeNext",
+    "noEmitOnError": true,
+    "noFallthroughCasesInSwitch": true,


ditto here - there's an eslint rule for this
i'd rather not use TS for linting if we can avoid it

Yeah same comment

jakebailey · 2024-05-07T16:21:30Z

Tested this out locally and it does seem to work; tsc -b at the root does the right thing (awesome), and opening up typescript-estree/src/index.ts and doing a find-all-refs on getCanonicalFileName finds the other projects, including the use in semanticInfo-singleRun.test.ts which it previously couldn't find until that was opened.

jakebailey

Re-looking at the config layout, I do think you can get away with fewer configs, actually, via some restructuring. But it technically already functions now, of course.

jakebailey · 2024-05-07T18:50:19Z

packages/ast-spec/tsconfig.json

+  "files": [],
+  "references": [
+    {
+      "path": "./tsconfig.build.json"
+    },
+    {
+      "path": "./tsconfig.spec.json"
+    }
+  ]


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

jakebailey · 2024-05-07T18:51:31Z

packages/ast-spec/tsconfig.json

-  "extends": "./tsconfig.build.json",
+  "extends": "../../tsconfig.base.json",
  "compilerOptions": {
-    "composite": false,
-    "rootDir": "."
+    "outDir": "../../dist/out-tsc/ast-spec"


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?

jakebailey · 2024-05-07T18:53:01Z

packages/ast-spec/tsconfig.build.json

+    "rootDir": "src",
+    "outDir": "dist",
+    "emitDeclarationOnly": false,


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.

jakebailey · 2024-05-07T18:56:13Z

tsconfig.json

+  "references": [
+    {
+      "path": "./packages/ast-spec"
+    },
+    {
+      "path": "./packages/eslint-plugin"
+    },


One gotcha here may be that TS won't do rebuilds in some situations if all projects are not listed here. Right now, you're using this top-level "solution" that points to other "solutions". Related is: microsoft/TypeScript#30608 / microsoft/TypeScript#55339

jakebailey · 2024-05-07T18:57:19Z

tsconfig.base.json

+    "experimentalDecorators": false,
+    "forceConsistentCasingInFileNames": true,


A lot of these settings are already their defaults and could be removed for clarity, e.g. allowJs, decorators, pretty, and so on.

JamesHenry force-pushed the nx-ts-plugin-2 branch from 3676ed3 to a200c2f Compare May 2, 2024 21:32

JamesHenry changed the title ~~wip~~ chore: refactor tsconfigs to use project references May 2, 2024

bradzacher reviewed May 2, 2024

View reviewed changes

chore: refactor tsconfigs to use project references

81d9f4f

JamesHenry force-pushed the nx-ts-plugin-2 branch from a200c2f to 81d9f4f Compare May 3, 2024 13:40

chore: latest nx 19 stable

98923ad

jakebailey reviewed May 7, 2024

View reviewed changes

auvred mentioned this pull request May 17, 2024

chore: use recommended config for Node.js 18 in TSConfig #9113

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: refactor tsconfigs to use project references #9038

chore: refactor tsconfigs to use project references #9038

JamesHenry commented May 2, 2024

typescript-eslint bot commented May 2, 2024

netlify bot commented May 2, 2024 •

edited

nx-cloud bot commented May 2, 2024 •

edited

codecov bot commented May 2, 2024 •

edited

bradzacher May 2, 2024

JamesHenry May 3, 2024

bradzacher May 2, 2024

JamesHenry May 3, 2024

bradzacher May 2, 2024

JamesHenry May 3, 2024

jakebailey commented May 7, 2024

jakebailey left a comment

jakebailey May 7, 2024

jakebailey May 7, 2024

jakebailey May 7, 2024

jakebailey May 7, 2024

jakebailey May 7, 2024

		export { visitorKeys } from './visitor-keys';
		export type { VisitorKeys } from './visitor-keys';

		"experimentalDecorators": false,
		"forceConsistentCasingInFileNames": true,

chore: refactor tsconfigs to use project references #9038

Are you sure you want to change the base?

chore: refactor tsconfigs to use project references #9038

Conversation

JamesHenry commented May 2, 2024

typescript-eslint bot commented May 2, 2024

netlify bot commented May 2, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented May 2, 2024 • edited

☁️ Nx Cloud Report

codecov bot commented May 2, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebailey commented May 7, 2024

jakebailey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented May 2, 2024 •

edited

nx-cloud bot commented May 2, 2024 •

edited

codecov bot commented May 2, 2024 •

edited