Skip to content

Repo: address issues introduced by updated nx configuration #11229

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

Open
4 tasks
aryaemami59 opened this issue May 20, 2025 · 10 comments · May be fixed by #11230
Open
4 tasks

Repo: address issues introduced by updated nx configuration #11229

aryaemami59 opened this issue May 20, 2025 · 10 comments · May be fixed by #11230
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@aryaemami59
Copy link
Contributor

Suggestion

Overview

There are several issues related to inferred plugin options, unexpected overrides, and missing configuration context in the current setup. This issue summarizes and clarifies those points while referencing this video made by @JamesHenry.


1. Plugin Option Overriding

  • Some inferred plugins provide default options that unintentionally override explicitly set options from other plugins.

For example, @nx/vite/plugin has a default typecheckTargetName of "typecheck", which overrides the targetName we explicitly set for @nx/js/typescript.

Initially, this was mitigated by explicitly overriding the value:

{
  "plugin": "@nx/vite/plugin",
  "include": ["packages/*"],
  "options": {
    "typecheckTargetName": "vite:typecheck"
  }
}

However, the current config no longer specifies that override:

{
  "plugin": "@nx/js/typescript",
  "options": {
    "typecheck": {
      "targetName": "typecheck"
    }
  }
},
{
  "plugin": "@nx/vite/plugin",
  "options": {
    // Implicit: "typecheckTargetName": "typecheck"
  }
}

This results in the @nx/vite/plugin inferring a typecheck target by detecting vitest.config.mts, which overrides the one from @nx/js/typescript.

You can clearly see this misalignment in the output from nx graph:

Current (Incorrect):

Incorrect graph

Expected (Correct):

Correct graph

This is a serious issue since running tsc -p tsconfig.json --noEmit ends up type-checking nothing, because our tsconfig.json uses empty files and include arrays, relying instead on project references. You can confirm this by introducing a type error and rerunning typecheck.


2. @nx/eslint:lint – Config Path Was Correct

  • In the video (14:13), it's mentioned that the value of options.eslintConfig (set to {workspaceRoot}/eslint.config.mjs) is incorrect. However, this was in fact correct. We only have a single eslint.config.mjs file, and it's at the root of the workspace.

3. Type Tests and Why utils:test Depends on build and typecheck

  • At 19:45, the video questions why utils:test depends on ^build. This dependency, along with typecheck, was actually intentional and necessary for running type-tests using vitest. These tests require the workspace's dependencies to be built beforehand, hence the original configuration:
"test": {
  "dependsOn": ["^build", "typecheck"]
}

Currently, only a few packages (e.g., ast-spec, utils) include type tests. In those workspaces:

  • typecheck.enabled was previously set to true in vitest.config.mts.
  • The typecheck.tsconfig was previously set to the local tsconfig.spec.json.

While vitest's type-checker is essentially a wrapper around tsc, there are some differences, which is why the scope of files to check was narrowed in the root vitest.config.base.mts file.


4. Missing --config=vitest.config.mts Flag

  • In the PR that simplified the configuration, the --config=vitest.config.mts flag was not carried over into the updated nx.json. While not strictly required, re-adding it would avoid vitest's config file lookup and offer a slight performance improvement.

Additional Info

I will submit a PR for this shortly.

@aryaemami59 aryaemami59 added triage Waiting for team members to take a look repo maintenance things to do with maintenance of the repo, and not with code/docs labels May 20, 2025
@JamesHenry
Copy link
Member

  1. This was me being dumb thinking that missing the config off would not create the target, rather than fallback to the default name.

I am discussing with the team about supporting a way to explicitly disabled an inferred target, because obviously this vite:typecheck target isn't referenced anywhere and so having it is misleading.


  1. You're right that I should have verified the presence of eslint configs in the projects, this is an uncommon setup and is fundamentally incompatible with the inference plugin because it is inferred based on the presence of the config file. We should add project level configs.

The mixing of executor and inference plugin points I made still stand, the setup did not make sense before.

@JamesHenry
Copy link
Member

  1. I'm still not clear on, the tsconfig.spec.json is included in the solution tsconfig.json and is therefore covered by the typecheck target from the typescript plugin?

@JamesHenry
Copy link
Member

  1. I am very skpetical about a negligible performance improvement there 😅

@aryaemami59
Copy link
Contributor Author

  1. This was me being dumb thinking that missing the config off would not create the target, rather than fallback to the default name.

I am discussing with the team about supporting a way to explicitly disabled an inferred target, because obviously this vite:typecheck target isn't referenced anywhere and so having it is misleading.

I think I got it, seems like all We have to do is: "typecheckTargetName": false.

  1. You're right that I should have verified the presence of eslint configs in the projects, this is an uncommon setup and is fundamentally incompatible with the inference plugin because it is inferred based on the presence of the config file. We should add project level configs.

Yeah, I'll add project level configs.

The mixing of executor and inference plugin points I made still stand, the setup did not make sense before.

I agree, I won't be adding anymore executors, We'll stick with one or the other.

@JamesHenry
Copy link
Member

I think I got it, seems like all We have to do is: "typecheckTargetName": false.

@aryaemami59 No, sadly that simply sets up a target called "false" and makes it do the typechecking, so it's confusing either way, so probably reverting it to the vite:typecheck is best for now, thanks!

I'll keep chatting with the team about addressing this

@JamesHenry
Copy link
Member

If the @typescript-eslint/triage-team really prefers not having the project level configs which simply inherit from the root one, we could remove the eslint inference plugin and simply have lint scripts inline on the projects, but yeah I think having the project level configs is clearer about the intent to run linting on a project level personally

@aryaemami59 aryaemami59 linked a pull request May 21, 2025 that will close this issue
3 tasks
@kirkwaiblinger
Copy link
Member

My only question with the project-level configs is - won't the root eslint config still apply to the projects/workspaces when executing eslint? Or does nx infer a lint task for the root that excludes subprojects that have an eslint config?

@kirkwaiblinger kirkwaiblinger added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels May 28, 2025
@aryaemami59
Copy link
Contributor Author

@kirkwaiblinger

My only question with the project-level configs is - won't the root eslint config still apply to the projects/workspaces when executing eslint? Or does nx infer a lint task for the root that excludes subprojects that have an eslint config?

The project level lint script will be some thing like:

eslint --config=${PROJECT_CWD}/eslint.config.mjs ${INIT_CWD}

@kirkwaiblinger
Copy link
Member

My concern is that executing that verbatim will lint the packages/**/* as well unless we exclude them, which duplicates work with the package-level tasks. We used to have an ignore pattern for that, see

"lintFilePatterns": ["{workspaceRoot}/!packages"]

(changed in https://github.com/typescript-eslint/typescript-eslint/pull/11226/files#diff-7680eb66f8e7b2155dfc25c4aa16a606a8fd3decb988265f218f3e11a8afa2beL20)

and before that,

"command": "eslint . --ignore-pattern=packages --cache"

(changed in https://github.com/typescript-eslint/typescript-eslint/pull/11135/files#diff-7680eb66f8e7b2155dfc25c4aa16a606a8fd3decb988265f218f3e11a8afa2beL12-L13)

@aryaemami59
Copy link
Contributor Author

@kirkwaiblinger

My concern is that executing that verbatim will lint the packages/**/* as well unless we exclude them, which duplicates work with the package-level tasks. We used to have an ignore pattern for that, see

typescript-eslint/project.json

Line 20 in 1c0e1ae

"lintFilePatterns": ["{workspaceRoot}/!packages"]
(changed in https://github.com/typescript-eslint/typescript-eslint/pull/11226/files#diff-7680eb66f8e7b2155dfc25c4aa16a606a8fd3decb988265f218f3e11a8afa2beL20)

and before that,

typescript-eslint/project.json

Line 12 in b2be3dc

"command": "eslint . --ignore-pattern=packages --cache"
(changed in https://github.com/typescript-eslint/typescript-eslint/pull/11135/files#diff-7680eb66f8e7b2155dfc25c4aa16a606a8fd3decb988265f218f3e11a8afa2beL12-L13)

Here is what they would look like:

In our nx.json We have these defaults.

Our root lint task

Our root lint script

Our project-level lint task

Our project-level lint script

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants