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

feat: add pnpm as optional package manager #1020

Merged
merged 1 commit into from
Nov 12, 2023
Merged

feat: add pnpm as optional package manager #1020

merged 1 commit into from
Nov 12, 2023

Conversation

GeoffreyBooth
Copy link
Member

This adds support for pnpm like we have for Yarn, resolving #1017. It also adds pnpm itself to the list of packages to test, though I don’t know if it will pass in all our environments. That’s the second commit and can be excluded if we want.

@codecov-commenter
Copy link

codecov-commenter commented Oct 22, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (eae7346) 96.44% compared to head (1ae5c4b) 96.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1020      +/-   ##
==========================================
- Coverage   96.44%   96.32%   -0.13%     
==========================================
  Files          28       28              
  Lines        2139     2177      +38     
==========================================
+ Hits         2063     2097      +34     
- Misses         76       80       +4     
Files Coverage Δ
lib/bin/citgm-all.js 95.35% <100.00%> (+0.01%) ⬆️
lib/bin/citgm.js 100.00% <100.00%> (ø)
lib/citgm.js 100.00% <100.00%> (ø)
lib/common-args.js 100.00% <100.00%> (ø)
lib/grab-project.js 96.12% <100.00%> (+0.12%) ⬆️
lib/package-manager/get-executable.js 100.00% <100.00%> (ø)
lib/package-manager/index.js 100.00% <100.00%> (ø)
lib/package-manager/test.js 99.37% <100.00%> (+0.01%) ⬆️
lib/lookup.js 91.07% <33.33%> (-0.83%) ⬇️
lib/package-manager/install.js 96.91% <88.88%> (-1.06%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@GeoffreyBooth
Copy link
Member Author

Should I keep pnpm in the lookup.json in this PR or separate it? How can I test that it passes in all the environments that we run CITGM before I land it?

@targos
Copy link
Member

targos commented Oct 23, 2023

Should I keep pnpm in the lookup.json in this PR or separate it?

Let's keep it. It's a good way to test the new code paths.

How can I test that it passes in all the environments that we run CITGM before I land it?

A good first step before trying all environments is to start https://github.com/nodejs/citgm/actions/workflows/test-module.yml

https://github.com/nodejs/citgm/actions/runs/6612820965

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM but don't merge until we're sure pnpm passes (or is skipped) on all supported platforms.

@targos
Copy link
Member

targos commented Oct 23, 2023

pnpm tests don't pass.

Copy link

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

this looks excellent. Thank you!

@GeoffreyBooth
Copy link
Member Author

Let's keep it. It's a good way to test the new code paths.

Well or we could add Next, that would also test the code paths. @Ethan-Arrowood what would the best commands be to build and test Next within CITGM?

@Ethan-Arrowood
Copy link

Not sure - I actually don't work on Next.js at all. But I will surface this with them. May not get a response until later as they are about to launch Next.js Conf and its all-hands-on-deck for that.

@GeoffreyBooth
Copy link
Member Author

I opened https://github.com/orgs/pnpm/discussions/7239 to ask the pnpm team about this.

@targos
Copy link
Member

targos commented Oct 31, 2023

@GeoffreyBooth
Copy link
Member Author

nodejs/citgm/actions/runs/6612820965

How exactly do you run these? I see “Run workflow,” what settings are you choosing?

@targos
Copy link
Member

targos commented Nov 2, 2023

You can see the parameters logged here: https://github.com/nodejs/citgm/actions/runs/6612820965/job/18230215140

@GeoffreyBooth
Copy link
Member Author

So like this?

image

Why are there separate settings for CITGM branch and ref? Aren’t those the same thing?

Anyway we seem to be making progress; it’s passing in some platforms/versions now: https://github.com/nodejs/citgm/actions/runs/6748209270

@targos
Copy link
Member

targos commented Nov 4, 2023

The first param (branch) should usually stay as main. It's the branch to use for the workflow file itself.

@GeoffreyBooth
Copy link
Member Author

Based on https://github.com/orgs/pnpm/discussions/7239#discussioncomment-7474442 it might take awhile before pnpm has a test suite that is consistently passing in CITGM. Perhaps I should remove the second commit from this PR, that adds pnpm to lookup.json, and just land the core support for pnpm as a package manager in CITGM? Then we can at least add other packages that depend on pnpm, such as Next, and add pnpm as a module under test whenever it’s ready.

@targos
Copy link
Member

targos commented Nov 7, 2023

SGTM

@GeoffreyBooth
Copy link
Member Author

@targos I’ve dropped the lookup.json commit. Do you want to land this? Then I can open a second PR with that other commit, and it can stay open until we get a usable test suite for pnpm.

@targos targos merged commit 46d35ae into main Nov 12, 2023
7 of 8 checks passed
@targos targos deleted the pnpm branch November 12, 2023 07: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.

4 participants