Conversation
|
No idea why Node.js 20.0 seems unable to use the Nock files from 16.x, but only on Windows 🤔 I think I'll just remove the 16.x tests, since it's EOL on Sep 11 anyway ... Edit: Actually it worked after a restart - I think a timeout caused some requests to not run, which messed with how Nock answers the http requests. |
Thanks for keeping those! That will help as we migrate support for multiple versions of corepack 👍 |
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
Can you please update the doc in the README?
sources/corepackUtils.ts
Outdated
| ? stream.pipe(createHash(build[0])) | ||
| : null; | ||
|
|
||
| const hash = stream.pipe(createHash(build[0] ?? `sha256`)); |
There was a problem hiding this comment.
I suggest we use sha224 because it's shorter
| const hash = stream.pipe(createHash(build[0] ?? `sha256`)); | |
| const hash = stream.pipe(createHash(build[0] ?? `sha224`)); |
There was a problem hiding this comment.
I'd have a small preference for sha256 because it's more well-known. I suspect most people seeing sha224 will wonder whether it's less secure and perhaps document themselves about it. sha256 seems less friction for users.
There was a problem hiding this comment.
It's easy to change if folks complain about it, but I suspect most folks won't pay attention to it – unless they're into software security, in which case I have the feeling they'd know what sha224 is. Until anyone complains sha224 seems a better choice for me, no strong feelings though
There was a problem hiding this comment.
sha224 seems a better choice for me
Can you explain why?
There was a problem hiding this comment.
Because it's shorter, and more than strong enough for what that hash is used for.
There was a problem hiding this comment.
My intuition is that the extra characters won't change a lot the experience, but sha256 is more well-known. I kept it as-is for this iteration.
|
Should be good to merge. |
PR-URL: #49486 Refs: nodejs/corepack#291 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #49486 Refs: nodejs/corepack#291 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#49486 Refs: nodejs/corepack#291 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
3 participants
This PR is the first step towards implementing the changes from #274
corepack install -g [--all] ...corepack installcorepack use [pattern]corepack upcorepack pack ...I added tests for
useandupsince those are new;installandpackshould already covered by the existing tests, which I migrated fromprepare/hydrateto the new commands.The old commands are still there, but I removed their help messages so they don't appear in
corepack -hanymore.