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

Switch to pnpm #1404

Merged
merged 1 commit into from
Jul 12, 2022
Merged

Switch to pnpm #1404

merged 1 commit into from
Jul 12, 2022

Conversation

akellbl4
Copy link
Collaborator

@akellbl4 akellbl4 commented Jul 1, 2022

installation time node_modules size
NPM i 10.79s 451M
PNPM 2.49s 342M

It's okay that size job is failing. It's because it can't install dependencies on master without pnpm-lock.yml

@akellbl4 akellbl4 force-pushed the switch-to-pnpm branch 2 times, most recently from 36c3fea to 6e0aca7 Compare July 5, 2022 19:49
@akellbl4 akellbl4 requested a review from Mavrin July 10, 2022 21:34
@akellbl4 akellbl4 marked this pull request as ready for review July 10, 2022 21:34
@akellbl4 akellbl4 requested a review from umputun as a code owner July 10, 2022 21:34
@codecov
Copy link

codecov bot commented Jul 10, 2022

Codecov Report

Merging #1404 (b58fa7e) into master (fddf1e2) will increase coverage by 0.17%.
The diff coverage is 80.64%.

❗ Current head b58fa7e differs from pull request most recent head 1cab9b0. Consider uploading reports for the commit 1cab9b0 to get more accurate results

@@            Coverage Diff             @@
##           master    #1404      +/-   ##
==========================================
+ Coverage   58.00%   58.17%   +0.17%     
==========================================
  Files         131      131              
  Lines        2900     2905       +5     
  Branches      699      735      +36     
==========================================
+ Hits         1682     1690       +8     
+ Misses       1214     1211       -3     
  Partials        4        4              
Impacted Files Coverage Δ
frontend/app/components/comment/comment.tsx 62.22% <ø> (-0.17%) ⬇️
frontend/app/components/select/select.tsx 92.30% <ø> (ø)
...ntend/app/components/comment-form/comment-form.tsx 51.70% <64.28%> (ø)
...pp/components/comment-form/comment-form.persist.ts 94.11% <94.11%> (ø)
frontend/app/components/root/root.tsx 0.00% <0.00%> (ø)
frontend/app/store/user/actions.ts 85.88% <0.00%> (+0.69%) ⬆️
frontend/app/store/comments/actions.ts 56.57% <0.00%> (+3.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fddf1e2...1cab9b0. Read the comment docs.

This was referenced Jul 11, 2022
Mavrin
Mavrin previously approved these changes Jul 11, 2022
RUN if [ -z "$SKIP_FRONTEND_BUILD" ] ; then \
CI=true npm ci --loglevel warn \
npm i -g pnpm && \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can use https://nodejs.org/api/corepack.html for activate pnpm
corepack pnpm install

Copy link
Collaborator Author

@akellbl4 akellbl4 Jul 11, 2022

Choose a reason for hiding this comment

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

Hm, I think its too early for corepack. It was introduced in v18 and it's experimental.
But worth considering when we move towards update to node18

UPD: Oh, I'm wrong it's available in v16.

umputun
umputun previously approved these changes Jul 11, 2022
@akellbl4 akellbl4 merged commit 2e777ea into master Jul 12, 2022
@akellbl4 akellbl4 deleted the switch-to-pnpm branch July 12, 2022 03:13
@paskal paskal added this to the v1.10.2 milestone Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants