-
Notifications
You must be signed in to change notification settings - Fork 136
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
chore: upgrade argon2 #431
Conversation
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected] |
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.
Since argon2
is a devDependency, it's not related to #421
That said, its fine to upgrade but it looks like this is causing code coverage to drop which makes me think its no longer testing the code path for argon in special-cases.ts
nft/src/utils/special-cases.ts
Lines 21 to 26 in bd68351
argon2({ id, emitAssetDirectory }) { | |
if (id.endsWith('argon2/argon2.js')) { | |
emitAssetDirectory(resolve(dirname(id), 'build', 'Release')); | |
emitAssetDirectory(resolve(dirname(id), 'prebuilds')); | |
emitAssetDirectory(resolve(dirname(id), 'lib', 'binding')); | |
} |
And more importantly, the zeromq-node-gyp
test started failing
https://github.com/vercel/nft/actions/runs/9686185024/job/26738120405?pr=431#step:10:1098
Could you clarify your comment from #407 (comment)? I thought you were saying the only reason |
@benmccann My comment was about replacing |
Ah, I see. Thanks for clarifying. I'll go ahead and close this then since it won't help with my goal of resolving #421 |
ref #421. I think this should unblock that issue, but I believe there is more required. Would you be able to fix it as a follow up or at least provide some pointers?