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] Move to pnpm #3192

Merged
merged 10 commits into from
Apr 10, 2024
Merged

[Tech] Move to pnpm #3192

merged 10 commits into from
Apr 10, 2024

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Nov 4, 2023

This involved

  • specifying our Electron dependency properly (we were missing the "github:" protocol)
  • adding some missing '@types' packages (and in turn fixing some type issues that weren't caught before)
  • fixing imports from 'react-router' (it's 'react-router-dom')
  • installing pnpm in Workflow runs
  • "Find and replace"ing quite a few "yarn"s with "pnpm"s

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:testing This PR is in testing, don't merge. label Nov 4, 2023
@CommandMC CommandMC self-assigned this Nov 4, 2023
@CommandMC CommandMC force-pushed the tech/pnpm branch 3 times, most recently from 808e49d to 98b2193 Compare November 4, 2023 00:54
@CommandMC CommandMC mentioned this pull request Nov 5, 2023
4 tasks
`copySync` never accepted a `recursive` parameter. Not sure how that didn't
cause an error before
Electron-Builder and pnpm are not the best of friends, but this should make them
 work together while not sacrificing too many pnpm speedups
@CommandMC CommandMC force-pushed the tech/pnpm branch 2 times, most recently from 36360f4 to 396e1d0 Compare March 29, 2024 11:14
@CommandMC CommandMC added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:testing This PR is in testing, don't merge. labels Mar 29, 2024
Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested here and is working fine.
would just be good to double check if AppVeyor has pnpm installed on their docker images already, otherwise the windows release will fail.

@flavioislima
Copy link
Member

Perhaps lets keep using npm or yarn on AppVeyor since it is just one of the builds and all the rest is working fine.

@CommandMC
Copy link
Collaborator Author

Using something other than pnpm would still leave us without a lock file, but I guess that's better than it not working at all

This should give us the best of both worlds, with working lockfile for packages
but also a working electron-builder
@CommandMC
Copy link
Collaborator Author

Silly meme

@CommandMC CommandMC merged commit 76e63a9 into main Apr 10, 2024
10 checks passed
@CommandMC CommandMC deleted the tech/pnpm branch April 10, 2024 19:33
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators Apr 10, 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.

2 participants