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

[Tech] Don't compute log file locations when importing constants.ts #3941

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CommandMC
Copy link
Collaborator

Log file locations (currentLogFile, lastLogFile, legendaryLogFile, etc.) were computed when importing the constants.ts file, which in turn lead to a lot of code being ran & modules being imported, even if those values are never actually read.
We can instead just use the common initXXX pattern to set the log file paths, and getLogFile to get them where they're needed.

This doesn't have an immediate benefit, but makes testing slightly easier in the future. Instead of having to mock a lot of modules just to get a non-crashing test setup, you now need slightly less

I've tested all aspects of the app relating to log files. Code itself was not touched, so I only tested on Linux (since the only thing required to test was whether the functions actually ran at the correct time)


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)

@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Aug 11, 2024
@CommandMC CommandMC requested a review from a team August 11, 2024 23:39
@CommandMC CommandMC self-assigned this Aug 11, 2024
@CommandMC CommandMC requested review from arielj, flavioislima, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team August 11, 2024 23:39
@arielj
Copy link
Collaborator

arielj commented Aug 12, 2024

I've been thinking about maybe splitting that big constants.ts file into smaller files inside a constants directory, then we could import the constants that we need in different places without node having to parse the whole constants file?

then in production code we'll still have some code importing constants/loggerPaths.ts and that would trigger the initialization of those paths as soon as it's imported, but then during testing if we test something that doesn't import anything from that file we don't have to worry?

Not sure why I have this feeling that the initX pattern is going to create some problem in the future (like we'll end up with things initialized at the wrong time/place). I haven't tried yet but I think we could solve the mocking thing and skip the need of initX?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

2 participants