-
Notifications
You must be signed in to change notification settings - Fork 27
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 nodejs/nan to 2.22 #303
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Thanks for the PR @benminer! Are you able to sign the CLA? |
Also please see the conventional commits check and update the commit/PR |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #303 +/- ##
=======================================
Coverage 41.92% 41.92%
=======================================
Files 14 14
Lines 2092 2092
Branches 42 42
=======================================
Hits 877 877
Misses 1197 1197
Partials 18 18 ☔ View full report in Codecov by Sentry. |
Hey @aabmass - thanks for checking in. I did, but looks like it went under my default Gmail account rather than the address I committed with. Fixed now -
For sure, should be good now! |
Thanks! I'll try to enable Node 21 tests and see what happens. There were some test failures #283 idk if you have any idea on that? |
I'll check it out! |
This PR upgrades
nan
to version 2.22, ensuring compatibility on NodeJS 21 and greater.The main culprit when building
pprof
wasSetAccessor
being renamed, see here: nodejs/nan@6bd62c9npm run test
: ✅system-test/system_test.sh
: ✅