[core] Avoid `node` types in the built packages #17533

LukasTy · 2025-04-24T17:21:36Z

Removing the usage of node in the tsconfig.compilerOptions.types to avoid NodeJS types in the built packages.

The drawback of this approach is inconclusive JSDoc/IntelliSense.
Hovering over methods shows node JSDoc where available, while the built output will have a different API (i.e. setTimeout).

I don't know how to easily avoid having node types in the project config, since there are numerous usages of path, __dirname and similar things, especially in test files.

LukasTy · 2025-04-24T17:23:47Z

packages/x-data-grid-generator/tsconfig.build.json

@@ -8,7 +8,9 @@
    "noEmit": false,
    "emitDeclarationOnly": true,
    "outDir": "build/esm",
-    "rootDir": "./src"
+    "rootDir": "./src",
+    "skipLibCheck": true,


Without this compilation failed due to not finding node types in the exceljs lib.
I don't think it's that important to check this validity twice (typescript step already checks with node types defined).

But our users' compilation would fail as well, right? Is this the issue?

Yes, if I remember correctly, that's the actual issue. 👍
I think it shouldn't affect our end users, should it?
If they have node types in their config, it's going to be fine, if they don't - it's going to error out. 🤔

They would have exceljs as a transitive dependency, and potentially not have @types/node defined in their env. I'd expect it to error on their end if they have skipLibCheck set to false (which I'd encourage).

Well, in this case, they would already be getting this error with the current setup. 🤔
Or am I missing something? 🤷

Well yes, they'll be getting this error, but because our own package mandates it as well.

Yeah, sadly, we depend on a stale package that is causing multiple issues. 🙈

mui-bot · 2025-04-24T17:24:42Z

Deploy preview: https://deploy-preview-17533--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 3e40856

LukasTy · 2025-04-24T17:32:50Z

packages/x-data-grid/src/utils/cleanupTracking/TimerBasedCleanupTracking.ts

@@ -14,7 +14,7 @@ export class TimerBasedCleanupTracking implements CleanupTracking {

  register(object: any, unsubscribe: UnsubscribeFn, unregisterToken: UnregisterToken): void {
    if (!this.timeouts) {
-      this.timeouts = new Map<number, NodeJS.Timeout>();
+      this.timeouts = new Map<number, ReturnType<typeof setTimeout>>();


Given that we need specificity to have conclusive types, I would vote for reverting mui/mui-public#195 and using window.setTimeout with return type of number instead of relying on terse ReturnType<typeof setTimeout>, which would now return NodeJS.Timeout in IDE/JSDoc and number in built output.

This makes perfect sense ... I was wondering why we use it without the window prefix as well at some point

window and global/globalThis are different things and can be misleading. I think that is the main argument to not use things from window which are usually present in global.

Why would IDE's keep returning NodeJS.Timeout?

Because setTimeout exists on Node and in regular (not build) tsconfig, we list the node types to make linting happy (we use some node APIs in tests and scripts)

in principle for code that runs on different environments you write different tsconfig files. I don't think they necessary need to be separate packages. Our current build tools wouldn't work with it though

That is what we have now more than before this PR.
In IDE and typechecking, we use the tsconfig.json file, while type building relies on the tsconfig.build.json file. 🤔

I mean multiple tsconfig files that each include different parts of the code. Like e.g. the vite example where vite config has node types included and src doesn't.

@Janpot, I get the idea. 👍
I have briefly explored your idea:

"app" tsconfig targeting the source files and excluding the test files

"test" tsconfig targeting the whole project
But then there are issues about unresolved @mui/x-internals package imports. 🙈 🤷

Here is the diff that I've been trying out on @mui/x-charts package:

diff --git a/packages/x-charts/package.json b/packages/x-charts/package.json index 28d0d761c5..53084fed97 100644 --- a/packages/x-charts/package.json +++ b/packages/x-charts/package.json @@ -24,7 +24,7 @@ "charts" ], "scripts": { - "typescript": "tsc -p tsconfig.json", + "typescript": "tsc -b", "build": "pnpm build:node && pnpm build:stable && pnpm build:types && pnpm build:copy-files", "build:node": "node ../../scripts/build.mjs node", "build:stable": "node ../../scripts/build.mjs stable", diff --git a/packages/x-charts/tsconfig.app.json b/packages/x-charts/tsconfig.app.json new file mode 100644 index 0000000000..a8dc1124e0 --- /dev/null +++ b/packages/x-charts/tsconfig.app.json @@ -0,0 +1,10 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "composite": true, + "types": ["../../extends.d.ts", "@mui/material/themeCssVarsAugmentation"], + "skipLibCheck": true + }, + "include": ["src/**/*"], + "exclude": ["src/**/*.test.*", "src/**/*.spec.*"] +} diff --git a/packages/x-charts/tsconfig.json b/packages/x-charts/tsconfig.json index a31f6f717c..173a7687e2 100644 --- a/packages/x-charts/tsconfig.json +++ b/packages/x-charts/tsconfig.json @@ -1,14 +1,5 @@ { "extends": "../../tsconfig.json", - "compilerOptions": { - "types": [ - "@mui/internal-test-utils/initMatchers", - "@mui/material/themeCssVarsAugmentation", - "chai-dom", - "mocha", - "node" - ], - "skipLibCheck": true - }, - "include": ["src/**/*", "../../test/utils/addChaiAssertions.ts"] + "references": [{ "path": "./tsconfig.app.json" }, { "path": "./tsconfig.test.json" }], + "files": [] } diff --git a/packages/x-charts/tsconfig.test.json b/packages/x-charts/tsconfig.test.json new file mode 100644 index 0000000000..317c39c264 --- /dev/null +++ b/packages/x-charts/tsconfig.test.json @@ -0,0 +1,15 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "composite": true, + "types": [ + "@mui/internal-test-utils/initMatchers", + "@mui/material/themeCssVarsAugmentation", + "chai-dom", + "mocha", + "node" + ], + "skipLibCheck": true + }, + "include": ["src/**/*", "../../test/utils/addChaiAssertions.ts"] +}

Do you know what I could be missing? 🤔

In any case, I think it could be left for a follow-up, don't you agree? 🤔

yes, absolutely

michelengelen

LGTM ... but for this I would wait for a second review

extends.d.ts

This reverts commit d932f05.

[core] Avoid node types in the built packages

cd15e9f

LukasTy added core Covers general upkeep, such as minor refactoring, typo fixes, or structural changes. type: enhancement This is not a bug, nor a new feature scope: code-infra labels Apr 24, 2025

LukasTy self-assigned this Apr 24, 2025

LukasTy commented Apr 24, 2025

View reviewed changes

LukasTy marked this pull request as ready for review April 24, 2025 20:58

LukasTy requested review from a team April 24, 2025 20:58

michelengelen approved these changes Apr 28, 2025

View reviewed changes

Janpot reviewed Apr 28, 2025

View reviewed changes

extends.d.ts Outdated Show resolved Hide resolved

LukasTy added 2 commits April 30, 2025 15:05

Merge branch 'master' into avoid-node-types-in-built-output

6ebe6a5

Code review: Remove option of wildcard env variables

8c52925

LukasTy requested review from Janpot and JCQuintas May 1, 2025 10:38

LukasTy added 4 commits May 1, 2025 13:49

Merge branch 'master' into avoid-node-types-in-built-output

d69491f

Try reverting "skipLibCheck"

d932f05

Merge branch 'master' into avoid-node-types-in-built-output

7c826b9

Revert "Try reverting "skipLibCheck""

347b018

This reverts commit d932f05.

Janpot approved these changes May 2, 2025

View reviewed changes

LukasTy added 2 commits May 5, 2025 08:12

Code review: Jan - rename custom TS types file

6e28f3a

Merge branch 'master' into avoid-node-types-in-built-output

3e40856

LukasTy enabled auto-merge (squash) May 5, 2025 05:14

LukasTy merged commit f990678 into mui:master May 5, 2025
19 checks passed

LukasTy deleted the avoid-node-types-in-built-output branch May 5, 2025 06:18

michaldudak mentioned this pull request May 7, 2025

[core] perf: avoid calling clearTimeout & cancelAnimationFrame mui/base-ui#1876

Merged

Uh oh!

[core] Avoid node types in the built packages #17533

[core] Avoid node types in the built packages #17533

Uh oh!

Conversation

LukasTy commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukasTy Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mui-bot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Janpot May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michelengelen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[core] Avoid `node` types in the built packages #17533

[core] Avoid `node` types in the built packages #17533

LukasTy commented Apr 24, 2025 •

edited

Loading

LukasTy Apr 24, 2025 •

edited

Loading

mui-bot commented Apr 24, 2025 •

edited

Loading

Janpot May 1, 2025 •

edited

Loading