-
Notifications
You must be signed in to change notification settings - Fork 170
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
add corepack project install
command
#551
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just needs tests 👍
Added! Let me know if you think there's more I should check for - there's not much actual new-new code here so hopefully we're good. Also LMK if you have thoughts on the follow-ups I listed or if you think there should be others. I could get started on some of them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Co-authored-by: Steven <[email protected]>
Can you add some documentation please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with more documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm is always lowercase https://npmjs.com/package/npm#faq-on-branding
specifically: - assert that `stdout` doesn't contain the word "Error" - use the same npm/pnpm/yarn versions that are already used elsewhere, maybe some mocking is happening - add a license field - check for existence of node_modules/ms/package.json
Fixed the tests - they had been working at some point but seemed to have started failing downloading the versions of npm/pnpm/yarn I'd specified, so I just changed them to match versions that are used elsewhere in tests. Also fixed the NPM->npm casing issue. @anonrig if you could give another CI approval (I think) this is good to go! |
It looks like there are some linting errors to address. |
Closes #505
This adds a
corepack project install
command, which is roughly equivalent tocorepack enable && npm install
for npm projectscorepack enable && pnpm install
for pnpm projectscorepack enable && yarn install
for yarn projectsI was able to use it successfully by just running
yarn build
locally, then I could donode dist/corepack.js project install
to verify it works on the corepack repo itself, andnode ../corepack/dist/corepack.js project install
from various git repos next door.I wanted to open this up early to get feedback in case the direction isn't considered correct. If it's looking good, I will add tests along similar lines to the existing ones.
Some follow-ons (which I think could be implemented as separate PRs, but could be persuaded to roll them into this one)
--frozen-lockfile
and--no-frozen-lockfile
--lockfile-only
--no-lockfile
--production
--dev
--offline
--force
corepack project add left-pad
corepack project remove left-pad
CC @styfle and @aduh95 since you commented on the issue.