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

Lint performance degradation after 13.8.3 update #9076

Closed
anton-gorbikov opened this issue Feb 22, 2022 · 3 comments · Fixed by #9340
Closed

Lint performance degradation after 13.8.3 update #9076

anton-gorbikov opened this issue Feb 22, 2022 · 3 comments · Fixed by #9340
Assignees
Labels
outdated scope: linter Issues related to Eslint support in Nx type: bug

Comments

@anton-gorbikov
Copy link

Current Behavior

enforce-module-boundaries rule reads nx cache before linting each file causing a significant performance degradation
It's caused by this change: aefe782

Expected Behavior

In some cases we are calling eslint directly, not via nx for better performance (e.g. git pre-commit hook) passing only changed files. nx cache should be loaded once during such execution (as it was in previous versions).

Maybe we can introduce some flag to distinguish IDE server from the manual run (just an idea, not sure about the proper fix) ?

Steps to Reproduce

  1. node_modules/eslint/bin/eslint "{apps,libs}/**/*.{ts,html}" --quiet (it will take 2 hours for medium size project instead of 3 minutes before the change)

Reproducible in nx-examples repository, yet the difference is not that huge (1-2 seconds) because the number of files is relatively low. I was modifying node_modules/@nrwl/eslint-plugin-nx/src/rules/enforce-module-boundaries.js directly to compare the performance, so it's hard to create a PR, but the change was quite obvious:

// from
if (!global.projectGraph || !(0, runtime_lint_utils_1.isTerminalRun)()) {
// to
if (!global.projectGraph) {

Failure Logs

No failure log, only performance issue.

Environment

$ nx report

 >  NX   Report complete - copy this into the issue template

   Node : 16.14.0
   OS   : darwin x64
   npm  : 7.24.2
   
   nx : 13.8.3
   @nrwl/angular : 13.8.3
   @nrwl/cli : 13.8.3
   @nrwl/cypress : 13.8.3
   @nrwl/detox : undefined
   @nrwl/devkit : 13.8.3
   @nrwl/eslint-plugin-nx : 13.8.3
   @nrwl/express : undefined
   @nrwl/jest : 13.8.3
   @nrwl/js : undefined
   @nrwl/linter : 13.8.3
   @nrwl/nest : undefined
   @nrwl/next : undefined
   @nrwl/node : undefined
   @nrwl/nx-cloud : undefined
   @nrwl/react : undefined
   @nrwl/react-native : undefined
   @nrwl/schematics : undefined
   @nrwl/storybook : 13.8.3
   @nrwl/tao : 13.8.3
   @nrwl/web : undefined
   @nrwl/workspace : 13.8.3
   typescript : 4.5.5
   rxjs : 6.5.5
   ---------------------------------------
   Community plugins:
         @angular/animations: 13.2.3
         @angular/cdk: 13.2.3
         @angular/common: 13.2.3
         @angular/compiler: 13.2.3
         @angular/core: 13.2.3
         @angular/forms: 13.2.3
         @angular/material: 13.2.3
         @angular/platform-browser: 13.2.3
         @angular/platform-browser-dynamic: 13.2.3
         @angular/pwa: 0.1102.14
         @angular/router: 13.2.3
         @angular/service-worker: 13.2.3
         @ionic/angular: 5.6.6
         @angular-builders/custom-webpack: 13.0.0
         @angular-builders/dev-server: 7.3.3
         @angular-devkit/build-angular: 13.2.4
         @angular/cli: 13.2.4
         @angular/compiler-cli: 13.2.3
         @angular/language-service: 13.2.3
         @ionic/angular-toolkit: 3.1.1
         @storybook/angular: 6.4.13
@Serg-Mois
Copy link

Faced the same problem.

@FrozenPandaz FrozenPandaz added the scope: linter Issues related to Eslint support in Nx label Mar 14, 2022
@meeroslav
Copy link
Contributor

meeroslav commented Mar 15, 2022

We do not suggest using eslint manually from the terminal. If you run it only against changed files, changing the tag in one project would not affect the parent projects (while in reality it often would).

Adding the explicit check to distinguish if the command is run via terminal or using eslint was to make sure the project graph used for the linter was always up to date (otherwise it would be read initially and used stale throughout the IDE session).

I will check if there is a way we can distinguish EslintServer runs from manual runs and use that as a denominator.

@github-actions
Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated scope: linter Issues related to Eslint support in Nx type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants