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

Pnpm #1044

Merged
merged 12 commits into from
Nov 1, 2023
Merged

Pnpm #1044

merged 12 commits into from
Nov 1, 2023

Conversation

dwu359
Copy link
Contributor

@dwu359 dwu359 commented Oct 30, 2023

pnpm

What solution does this PR provide?

Switches package management from yarn to pnpm for frontend and serverless.

Also modified the dlp-cli so that it uses pnpm instead of yarn. Also add a --yarn flag in case if anyone is in the process of transitioning from yarn to pnpm.

Also modified node build check workflow to use pnpm instead of yarn.

Testing Methodology
Everything was able to be installed via pnpm.

Any other considerations
DSGT-DLP/dlp-cli#11

@dwu359 dwu359 requested a review from a team as a code owner October 30, 2023 03:12
@sweep-ai
Copy link
Contributor

sweep-ai bot commented Oct 30, 2023

Apply Sweep Rules to your PR?

  • Apply: Leftover TODOs in the code should be handled.
  • Apply: All new business logic should have corresponding unit tests in the tests/ directory.
  • Apply: Any clearly inefficient or repeated code should be optimized or refactored.

@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dwu359 dwu359 mentioned this pull request Oct 30, 2023
@karkir0003
Copy link
Member

@dwu359 can you add an explanation in the github issue (or pr) for why we want to use pnpm over yarn? have we ensured that pnpm can still install any node dependency we want to use in the future (at least for 90+% of our potential usecases).

Copy link
Member

@karkir0003 karkir0003 left a comment

Choose a reason for hiding this comment

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

address my comment @dwu359

@dwu359
Copy link
Contributor Author

dwu359 commented Oct 30, 2023

@dwu359 can you add an explanation in the github issue (or pr) for why we want to use pnpm over yarn? have we ensured that pnpm can still install any node dependency we want to use in the future (at least for 90+% of our potential usecases).

From what I've read, pnpm is slightly faster than yarn berry and uses up less space than yarn because it utilizes symlinks instead of directly copying packages. That did seem the case for my machine, but I'll need other ppl to try it out also if that is the case. Pnpm was able to install basically anything that I threw at it, since it installs from the npm registry.

@dwu359
Copy link
Contributor Author

dwu359 commented Oct 30, 2023

Actually if you look at the build times for https://github.com/DSGT-DLP/Deep-Learning-Playground/actions/workflows/node.js.yml, it does seem to be about 20s faster

@dwu359 dwu359 requested a review from karkir0003 October 30, 2023 14:38
@farisdurrani
Copy link
Member

Works on my laptop

Copy link

sonarcloud bot commented Nov 1, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dwu359 dwu359 added this pull request to the merge queue Nov 1, 2023
Merged via the queue into nextjs with commit 9057f21 Nov 1, 2023
11 checks passed
@dwu359 dwu359 deleted the pnpm branch November 1, 2023 02:14
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.

3 participants