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

Deprecated subdependencies found in @mapbox/node-pre-gyp #421

Open
cdwmhcc opened this issue May 23, 2024 · 10 comments
Open

Deprecated subdependencies found in @mapbox/node-pre-gyp #421

cdwmhcc opened this issue May 23, 2024 · 10 comments
Labels
good first issue Good for newcomers

Comments

@cdwmhcc
Copy link

cdwmhcc commented May 23, 2024

Description

When installing dependencies for our project that uses @vercel/nft, we encountered warnings about deprecated subdependencies. These warnings are caused by the @mapbox/node-pre-gyp package, which is a dependency of @vercel/nft.

Details

The following deprecated subdependencies are reported during pnpm install:

  • are-we-there-yet@2.0.0
  • gauge@3.0.2
  • inflight@1.0.6
  • npmlog@5.0.1

Expected Behavior

Dependencies within @vercel/nft should not rely on deprecated packages to ensure better security and compatibility with modern development environments.

Additional Information

  • Node.js version: [20.13.1]
  • pnpm version: [9.1.2]
  • Operating System: [windows 11]
@styfle
Copy link
Member

styfle commented May 23, 2024

There is some related discussion here: #407 (comment)

Would you like to submit a PR to fix it?

@benmccann
Copy link

benmccann commented Jun 26, 2024

I wonder if we even need to include the dependency at all. I looked at argon2 to see how its used and its just a single function export:

Compare https://npmgraph.js.org/?q=argon2 vs https://npmgraph.js.org/?q=argon2@0.31.1. The new version drops 55 dependencies including @mapbox/node-pre-gyp

@benmccann
Copy link

I sent #431 to upgrade argon2. I'm not sure what else is required to address this issue, but I believe this new version of argon2 should hopefully address the blockers and make this issue much easier to solve

@styfle
Copy link
Member

styfle commented Jun 27, 2024

Its not related to argon2 at all. I updated my comment from the other issue and I'll repost here.

I looked at how consumers are using it (for example, argon2) and its just a single function export:

const binary = require("@mapbox/node-pre-gyp");
const bindingPath = binary.find(path.resolve(__dirname, "./package.json"));

So perhaps we could implement this find() function easily assuming its just reading the package.json.

Do you want to submit a PR to implement this?

@styfle styfle added the good first issue Good for newcomers label Jun 27, 2024
@benmccann
Copy link

benmccann commented Jun 27, 2024

I see. That makes much more sense to me. Thanks!

It's not exactly clear to me that find is easily replaceable though it would be awesome if someone can figure out how to do that. In the meantime I'll work on cleaning up @mapbox/node-pre-gyp:

@benmccann
Copy link

argon2 switched from @mapbox/node-pre-gyp to node-gyp-build here:
ranisalt/node-argon2@b476028#diff-bc704b883867dea430073059d0e3061c8ac87c037826b06007646f667777916d

The latter library is dramatically lighter. I wonder if we could make the same switch here

@benmccann
Copy link

@styfle based on the link from my past comment, do you think something like this might work?

Screenshot from 2024-06-30 20-37-33

It looks like argon2 was able to do something similar, but when I create a dummy find method that returns null I don't get any extra failing tests (I have 6 that fail for me both on main and with a clearly broken implementation), so I'm not overly confident in the fix. I'm wholly unfamiliar with nft so don't know if I can put together and test a fix, but I hope that discovering the argon2 implementation uses node-gyp-build to do the same as @mapbox/node-pre-gyp's find will be enough to let you address this.

@styfle
Copy link
Member

styfle commented Jul 1, 2024

nft supports both node-gyp-build and @mapbox/node-pre-gyp because it needs to work on any version of any npm package

@vhovorun
Copy link

@styfle @benmccann Hey guys, sorry for pinging u but any updates on this one?

@benmccann
Copy link

I don't understand the changes that would need to be made here to remove the use of @mapbox/node-pre-gyp, but if mapbox/node-pre-gyp#829 is merged then it should fix the issue in @mapbox/node-pre-gyp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants