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.

Sign up for GitHub

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

Jump to bottom

chore: switch to pnpm #153

Merged
merged 7 commits into from
Jan 19, 2022
Merged

chore: switch to pnpm #153

merged 7 commits into from
Jan 19, 2022

Conversation

Shinigami92
Copy link
Member

No description provided.

@Shinigami92 Shinigami92 added the c: chore PR that doesn't affect the runtime behavior label Jan 15, 2022
@Shinigami92 Shinigami92 requested a review from a team January 15, 2022 12:37
@Shinigami92 Shinigami92 self-assigned this Jan 15, 2022
@netlify
Copy link

netlify bot commented Jan 15, 2022 •
edited
Loading

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: 9e83b3c

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e836ff8fb2430007d67b4f

😎 Browse the preview: https://deploy-preview-153--vigilant-wescoff-04e480.netlify.app

@Shinigami92
Copy link
Member Author

Behaviour copied from https://github.com/vitejs/vite/blob/ea615b07ce8c5dc55d24f293803c7f5886ce425b/netlify.toml and https://github.com/vitejs/vite/blob/ea615b07ce8c5dc55d24f293803c7f5886ce425b/package.json

@Shinigami92
Copy link
Member Author

Need to install npm-run-all

prisis
prisis reviewed Jan 15, 2022
View reviewed changes
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@damienwebdev
Copy link
Member

Why can't we just use what Node provides out of the box with npm do we really need pnpm?

@Shinigami92
Copy link
Member Author

Why can't we just use what Node provides out of the box with npm do we really need pnpm?

pnpm has MANY improvements over npm
It downloads the files in a global local cache + it uses symlinks to that
So your e.g. SSD will love you

npm is downloading everything always when you removed the node_modules folder and try to make a clean install

@prisis
Copy link
Member

prisis commented Jan 15, 2022

Wouldn't it be easier for new contributor to use yarn? If i'm not mistaken its comes with node

@Shinigami92
Copy link
Member Author

Wouldn't it be easier for new contributor to use yarn? If i'm not mistaken its comes with node

yarn has some benefits but is not well maintained
also it is driven by facebook where pnpm is community driven
also yarn v3 needs some more configuration where pnpm doesn't need anything
new contributors are directly informed and get telled that they need to just do npm install -g pnpm
pnpm is also in corepack compatible
yarn v1 is legacy and not maintained anymore but you need it to switch to v3
pnpm has a much better CLI

@prisis
Copy link
Member

prisis commented Jan 15, 2022 •
edited
Loading

Im pretty fine with pnpm, using it too in most of all my projects

prisis
prisis previously approved these changes Jan 15, 2022
View reviewed changes
@Shinigami92 Shinigami92 requested a review from a team January 15, 2022 20:19
import-brain
import-brain previously approved these changes Jan 17, 2022
View reviewed changes
@Shinigami92 Shinigami92 dismissed stale reviews from import-brain and prisis via 612a815 January 17, 2022 15:46
@Shinigami92 Shinigami92 added this to the v6.0.0 milestone Jan 17, 2022
@Shinigami92 Shinigami92 added the c: dependencies Pull requests that adds/updates a dependency label Jan 17, 2022
@Shinigami92
Copy link
Member Author

@prisis I rebase this branch, please re-approve it 🙂

import-brain
import-brain previously approved these changes Jan 17, 2022
View reviewed changes
@Shinigami92 Shinigami92 marked this pull request as draft January 18, 2022 22:22
@Shinigami92 Shinigami92 marked this pull request as ready for review January 18, 2022 22:32
import-brain
import-brain previously approved these changes Jan 19, 2022
View reviewed changes
damienwebdev
damienwebdev approved these changes Jan 19, 2022
View reviewed changes
Copy link
Member

@damienwebdev damienwebdev left a comment

Choose a reason for hiding this comment

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

lgtm

@Shinigami92 Shinigami92 merged commit a273f67 into main Jan 19, 2022
@Shinigami92 Shinigami92 deleted the switch-to-pnpm branch January 19, 2022 18:24
pkuczynski pushed a commit to pkuczynski/faker that referenced this pull request Jan 21, 2022
@Shinigami92 @pkuczynski
chore: switch to pnpm (faker-js#153)
1e48d79
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JessicaSachs JessicaSachs JessicaSachs approved these changes

@damienwebdev damienwebdev damienwebdev approved these changes

@import-brain import-brain import-brain approved these changes

@prisis prisis Awaiting requested review from prisis

Assignees

@Shinigami92 Shinigami92

Labels
c: chore PR that doesn't affect the runtime behavior c: dependencies Pull requests that adds/updates a dependency
Projects
None yet
Milestone
v6.0 - Ensure Project Foundation
Development

Successfully merging this pull request may close these issues.
5 participants
@Shinigami92 @damienwebdev @prisis @JessicaSachs @import-brain