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

chore: Switch to pnpm package manager #147

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

Conversation

gwhitney
Copy link
Collaborator

@gwhitney gwhitney commented Sep 1, 2022

NOTE: If you pull this PR and want to operate with it, you must install pnpm version 7.17.1 or later, and use only pnpm for package management (do not use npm while on this branch!).

Switches this project to exclusively using the pnpm package manager. Benefits include faster performance, reduced disk space usage if you have multiple projects using pnpm (they share dependency code), and the ability to use the more human-readable JSON5 format for the package manifest file. Note that once this is merged, neither npm or yarn will be able to mange the packages for this project; rather, all developers will be obliged to use pnpm.

Resolves #140.

@gwhitney
Copy link
Collaborator Author

The necessary updates to pnpm have been merged and are published in pnpm 7.17.1. This PR is ready for review/ merging whenever it becomes convenient.

@gwhitney gwhitney marked this pull request as ready for review November 27, 2022 13:50
@gwhitney
Copy link
Collaborator Author

Just to make sure everyone sees it, I will also post down here: if you are on this branch, you must use pnpm version 7.17.1 or later, not npm. So make sure you have the necessary version of pnpm before pulling this branch. Thanks.

@liammulh
Copy link
Member

My first instinct is to wait until the students are done, then merge, but we can discuss in today's meeting.

@gwhitney
Copy link
Collaborator Author

gwhitney commented Dec 1, 2022

With the latest commit to this PR, the duplicated section between the two tests in the ci.yaml specification for github actions has gotten rather long/intricate, and should be textually collapsed for maintenance purposes even if it doesn't actually reduce the server work. Should I add a commit here that does that, or wait until this is merged and file a separate PR? Thanks for your thoughts.

@liammulh
Copy link
Member

liammulh commented Dec 1, 2022

As long as we document the fact that we did some de-duplication of the YAML in the commit message that gets merged into main, I don't mind if you add the commit to this PR.

@gwhitney
Copy link
Collaborator Author

Sadly this will have to wait until after the TU Delft project, since it now seems clear the team will clone the repository with an npm-managed version, and we definitely don't want to switch during that project. Or in other words: curses, foiled again.

@gwhitney gwhitney marked this pull request as draft April 22, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to pnpm
2 participants