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] Use compile-time constants for isWindows, isMac and isLinux #3944

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CommandMC
Copy link
Collaborator

When building Heroic, we know which platform we're building for at all times (the current platform when running pnpm start, the target platform when running dist:win, dist:mac, etc.). Because of this, we can tell Vite about that platform (with a special mode), and in turn replace constants like isWindows with literal values (true/false). This then allows Rollup to do tree-shaking (it will completely eliminate code paths and even entire dependencies if they're not used)

Rollup sadly seems rather peculiar about when it'll do this tree-shaking:

if (false)
  console.error('this should not happen!')

gets removed as expected, but

switch (false) {
  case true:
    console.error('this should not happen!')
    break
}

does not. As such, we'll have to move towards if-based checks (if (isWindows) { /* ... */ } else if (isMac) { /* ... */ }) instead of switch-based checks (switch (process.platform) { case 'win32': /* ... */ }) to get the most out of this feature


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:wip WIP, don't merge. label Aug 12, 2024
@CommandMC CommandMC self-assigned this Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:wip WIP, don't merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant