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

Update lint.js now that we are using ESLint 9 #1484

Closed
samreid opened this issue Oct 11, 2024 · 13 comments
Closed

Update lint.js now that we are using ESLint 9 #1484

samreid opened this issue Oct 11, 2024 · 13 comments

Comments

@samreid
Copy link
Member

samreid commented Oct 11, 2024

From #1451 there were many TODOs to upgrade the lint process. Most of this should be done before #1415.

@zepumph
Copy link
Member

zepumph commented Oct 11, 2024

From #1469 here are some checklist items:

  • Should lint-all do multiple in one batch? Or separate? Do we still want progress bar? If we combine repos and use same monolithic cache, will it augment or overwrite?
  • grunt lint-everything no longer works with --chip-away
  • lint-all is taking longer

@samreid samreid removed their assignment Oct 14, 2024
samreid added a commit to phetsims/perennial that referenced this issue Oct 17, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Oct 23, 2024
@samreid
Copy link
Member Author

samreid commented Oct 31, 2024

I'll run performance tests of using the API or processes on cached or --clean, on mac and windows. That will help us know for sure whether we need both implementations.

@samreid samreid self-assigned this Oct 31, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 4, 2024
@samreid
Copy link
Member Author

samreid commented Nov 4, 2024

mac
cache is fresh - API only => 9sec
cache is fresh - spawn only => 52sec
--clean - spawn only => 4m22s
--clean - API only => 54% then crash at 2m40s

windows
cache is fresh - API only => 16sec
cache is fresh - spawn only => 63s
--clean - spawn only => 4m23s
--clean - API only => 54% then crash at 4m14s

@samreid samreid removed their assignment Nov 4, 2024
@zepumph
Copy link
Member

zepumph commented Nov 4, 2024

Design goals:

  1. No crashing possible
  2. Cleaner implementation than one file with two implementations

Next steps:

  1. Let's go all in on the Node CLI (API), and use sub processes to keep heaps for overloading. Lets start with 50 repos per sub process.
  2. We can run multiple sub processes at once (multi threading)
  3. We can remove the npx implementation
  4. Keep the CLI running a single repo at a time, with a cache file per repo.

@zepumph
Copy link
Member

zepumph commented Nov 4, 2024

  • I think we should keep the ability to have debug logging, maybe turned on via the --verbose flag? What do you think?
Subject: [PATCH] add authors, https://github.com/phetsims/chipper/issues/1502
---
Index: js/grunt/lint.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/grunt/lint.ts b/js/grunt/lint.ts
--- a/js/grunt/lint.ts	(revision 9c6461e0c47a003490903950a5c25b4448071083)
+++ b/js/grunt/lint.ts	(date 1730747840002)
@@ -175,7 +175,8 @@
     cache: true,
     cacheLocation: path.resolve( getCacheLocation( repo ) ),
     fix: options.fix,
-    errorOnUnmatchedPattern: false
+    errorOnUnmatchedPattern: false,
+    flags: [ '--debug' ]
   };
 
   // Create ESLint instance

samreid added a commit to phetsims/perennial that referenced this issue Nov 4, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 4, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
@samreid samreid removed their assignment Nov 5, 2024
@samreid samreid mentioned this issue Nov 5, 2024
1 task
@zepumph
Copy link
Member

zepumph commented Nov 5, 2024

  • Even before this patch, I was feeling like js/eslint/ was getting a bit unweildy. I think we should make a subdir called config/, and keep linting module and running code that the top level. SEE Refactorings for js/eslint/ perennial#399
  • This morning I fixed the "reopen issues with TODOs" task and it will likely yield an annoying merge conflict, let's merge main into the branches together.
  • I committed a change that removed cleanOnly and can just run a function. I'm unsure if cleaning tsc is needed from lint, but I like keeping it for now.
  • I ran grunt lint --clean --all and it took 2 minutes before this commit phetsims/perennial@8bc2e52
  • require( 'os').cpus().length -> 20 on MK windows, and 128 on sparky.

zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
@zepumph
Copy link
Member

zepumph commented Nov 5, 2024


 mjkauzmann ~/PHET/git/perennial-alias (lint-batches-chipper-1484)
 $ time grunt lint --all --clean
Running "lint" task
processes 4
Done.

real    2m18.486s
user    0m0.061s
sys     0m0.061s

 mjkauzmann ~/PHET/git/perennial-alias (lint-batches-chipper-1484)
 $ time grunt lint --all --clean
Running "lint" task
processes 15
Done.

real    1m56.170s
user    0m0.060s
sys     0m0.062s

zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
zepumph added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
@zepumph zepumph assigned samreid and unassigned zepumph Nov 5, 2024
@zepumph
Copy link
Member

zepumph commented Nov 5, 2024

I added 6 TODOs as part of the review. @samreid and I merged to main, and the rest is in his court. Thanks SR!1

samreid added a commit to phetsims/perennial that referenced this issue Nov 5, 2024
@samreid
Copy link
Member Author

samreid commented Nov 5, 2024

TODOs addressed, --processes added which sets the maximum number of allowable processes. We are merged to main. Feature branches deleted. Closing.

@samreid samreid closed this as completed Nov 5, 2024
@samreid
Copy link
Member Author

samreid commented Nov 5, 2024

On second thought, I'm very confused about all the entry points that are allowed to pass through LintOptions. The default occurs in several places. Would be good for @zepumph to review and comment.

@samreid samreid reopened this Nov 5, 2024
@samreid samreid assigned zepumph and unassigned samreid Nov 5, 2024
@zepumph zepumph removed their assignment Nov 7, 2024
@zepumph
Copy link
Member

zepumph commented Nov 8, 2024

@samreid and I agree it is awkward, and getLintOptions shouldn't always provide defaults that are duplicated with the module defaults. We will make a separate side issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants