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

[DX] Optimize ESLint includes #3518

Merged
merged 2 commits into from
May 18, 2024
Merged

[DX] Optimize ESLint includes #3518

merged 2 commits into from
May 18, 2024

Conversation

arielj
Copy link
Collaborator

@arielj arielj commented Feb 4, 2024

The current tsconfing.eslint.json config is overriding the include property telling eslint to include all files and not only the files we care (I understand it's trying to lint too much).

Without this change I can't push any code, the eslint process starts consuming RAM like crazy until Node crashed with a memory limit error after it gets to 8GB of ram.

With these changes it has no issues and it goes from like a minute of loading then crash to processing all the files in 11 seconds without crashing.

I can see with the --debug flag that it's still parsing all the files from the project that we care about (all ts/tsx/js files inside src and e2e) so this seems to be safe. Any feedback is appreciated, I'm not an expert on these configurations.


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@arielj arielj added the pr:ready-for-review Feature-complete, ready for the grind! :P label Feb 4, 2024
@arielj arielj requested review from a team, flavioislima, CommandMC, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team February 4, 2024 18:05
@CommandMC
Copy link
Collaborator

Hmm, I have a hard time reproducing this issue. I did get it once, but now can't trigger it again; deleted the cache of course

My original intent with the change (including all .js files in ESLint) was to lint "random" files like downloadCount.js. Now I'm however unsure if we even want that (not every one-off script needs perfect code, after all). Maybe removing the "**/*.js" from the original include array is the easier solution.

@arielj
Copy link
Collaborator Author

arielj commented Feb 5, 2024

Hmm, I have a hard time reproducing this issue. I did get it once, but now can't trigger it again; deleted the cache of course

For me it happens 100% of the times, with and without cache (it even freezes my whole system temporarily) :/

I've been changing that config locally to be able to push other branches

My original intent with the change (including all .js files in ESLint) was to lint "random" files like downloadCount.js. Now I'm however unsure if we even want that (not every one-off script needs perfect code, after all). Maybe removing the "**/*.js" from the original include array is the easier solution.

I guess we can add files manually if we want a file to be linted instead? to prevent this issue and still have linting (sometimes we might forget to add some file, but someone will notice eventually)

@Etaash-mathamsetty
Copy link
Member

I want to try this PR could you pls rebase?

@arielj
Copy link
Collaborator Author

arielj commented Apr 27, 2024

I want to try this PR could you pls rebase?

I'll do it later, but you only need this change https://github.com/Heroic-Games-Launcher/HeroicGamesLauncher/pull/3518/files#diff-5053c0bda6e3ba2fcfa5bbe118ecf5c60fa9f099b78b3e6deb66e5679424f515R3

@arielj
Copy link
Collaborator Author

arielj commented May 17, 2024

I removed the updates of the eslint stuff, some packages were already outdated but updating them makes them incompatible with other eslint packages and I didn't want to waste time figuring that out since it's not relevant for this fix.

The main thing is the "includes" value to limit the number of files that are processed

@CommandMC CommandMC changed the title [DX] Update eslint packages and optimize includes [DX] Optimize ESLint includes May 18, 2024
@CommandMC CommandMC merged commit 1eb0ad3 into main May 18, 2024
9 checks passed
@CommandMC CommandMC deleted the eslint-config-tweaks branch May 18, 2024 20:39
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators May 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants