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

fix(analyze): node-gyp-build not including zeromq prebuilds #392

Merged

Conversation

eulersson
Copy link
Contributor

@eulersson eulersson commented Feb 13, 2024

Three things I want to point out:

  1. In zeromq the way node-gyp-build is called is by passing path.join(__dirname, "..") instead of __dirname to the require("node-gyp-build")(...) call.
  2. In zeromq they use @aminya/node-gyp-build instead of node-gyp-build, particular-li version 4.5.0-aminya.5.
  3. The zeromq version that's recommended for new project is the 6 which is beta according to @aminya https://github.com/zeromq/zeromq.js.

After those changes it resolves OK.

❯ node ./node_modules/@vercel/nft/out/cli.js print ./node_modules/zeromq/lib/index.js
FILELIST:
node_modules/@aminya/node-gyp-build/index.js
node_modules/@aminya/node-gyp-build/package.json
node_modules/zeromq/lib/draft.js
node_modules/zeromq/lib/index.js
node_modules/zeromq/lib/native.js
node_modules/zeromq/lib/util.js
node_modules/zeromq/package.json
node_modules/zeromq/prebuilds/darwin-x64/node.napi.glibc.node <---- It worked!

…evel where de require() is run at. Fixes zeromq.js. Closes vercel#391.
@eulersson eulersson changed the title node-gyp-build not including zeromq prebuilds fix(resolution) node-gyp-build not including zeromq prebuilds Feb 13, 2024
@eulersson eulersson changed the title fix(resolution) node-gyp-build not including zeromq prebuilds fix(resolution): node-gyp-build not including zeromq prebuilds Feb 13, 2024
test/unit.test.js Outdated Show resolved Hide resolved
@eulersson
Copy link
Contributor Author

I will fix the test havoc ⛈️

package.json Outdated Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
src/analyze.ts Outdated Show resolved Hide resolved
@aminya
Copy link

aminya commented Feb 13, 2024

@eulersson Thanks for making a fix. Let me know if there is anything to do on the Zeromq side.

@eulersson
Copy link
Contributor Author

@eulersson Thanks for making a fix. Let me know if there is anything to do on the Zeromq side.

No problem. For now I do not think it will be needed.

@styfle styfle changed the title fix(resolution): node-gyp-build not including zeromq prebuilds fix(analyze): node-gyp-build not including zeromq prebuilds Feb 13, 2024
Copy link
Member

@styfle styfle left a comment

Choose a reason for hiding this comment

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

Great work, thanks! 🎉

@styfle styfle merged commit 0598a4c into vercel:main Feb 13, 2024
14 checks passed
Copy link

🎉 This PR is included in version 0.26.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

Add support for zeromq
3 participants