feat: Added Node 22 to prebuild list and tests

[bizob2828]

👋 Contrasters. We're trying to see what breaks in our CI for Node 22 and the good thing was it was only assertions around the output of this module. I think I did the right thing here, but let me know and I can include what's missing. I know Node 22 isn't LTS yet but you all know how that goes with customers. I have verified aside from running tests on Node 22 here that if I run npm pack and install the .tgz to our repo that the tests that were failing are now working 🎉

[bizob2828]

The failing mac builds are because the latest mac image in GHA breaks node gyp. We fixed it here. Also, win-2019 may no longer work and prob want to switch to windows-latest? I can fix this but probably should be a separate PR, no? Also judging by the recent commits looks like @bmacnaughton is a primary maintainer of this now?

[tough-griff]

Looks like Node 22 on Windows is failing--I don't have a machine to debug, maybe @bmacnaughton or @meandthemoon can try to build on 22 locally.

[bizob2828]

Looks like Node 22 on Windows is failing--I don't have a machine to debug, maybe @bmacnaughton or @meandthemoon can try to build on 22 locally.

We're hitting same issue on a native module on our end. I think this is node-gyp not updated to support node 22 fully. This PR may have to wait a bit

[bmacnaughton]

am going to give it a try. not sure the state of my windows setup for building, but will find out.

[bmacnaughton]

here is the problem. i haven't found a way around it yet - node-gyp-build@4.8.1 has a fix for this in it, but it still doesn't compile when running node v20.12.2. While node-gyp-build has fixed their version of the code, the same project on git (prebuild) has not yet fixed prebuildify where it fails with the same error once you install node-gyp-build@4.8.1.

It will compile running node 20.9.0, so my guess is the security fix has been backported to some node versions. But it looks like the most recent node-gyp main doesn't have this fix in it: https://github.com/nodejs/node-gyp/blob/323957b74e9586fb3fbfb2acad5040379c778de6/lib/build.js#L201, though I can't be sure because prebuildify fails before we get to it.

so, yeah, i guess we wait until the various pieces we depend on stop running .cmd and .bat files without a shell.

[bizob2828]

here is the problem. i haven't found a way around it yet - node-gyp-build@4.8.1 has a fix for this in it, but it still doesn't compile when running node v20.12.2. While node-gyp-build has fixed their version of the code, the same project on git (prebuild) has not yet fixed prebuildify where it fails with the same error once you install node-gyp-build@4.8.1.

It will compile running node 20.9.0, so my guess is the security fix has been backported to some node versions. But it looks like the most recent node-gyp main doesn't have this fix in it: https://github.com/nodejs/node-gyp/blob/323957b74e9586fb3fbfb2acad5040379c778de6/lib/build.js#L201, though I can't be sure because prebuildify fails before we get to it.

so, yeah, i guess we wait until the various pieces we depend on stop running .cmd and .bat files without a shell.

I'm not really following all of that @bmacnaughton. So node core fixed a CVE with child_process.spawn and node-gyp-build@4.8.1 has a fix but that fix doesn't work on node 20.12.2? What's the fix you're referring to, is it this one. I'm trying to figure out which OSS project I need to log this issue just to make sure it gets some 👀 on it

[bmacnaughton]

The OSS project that still needs fixed is prebuildify (same github project as node-gyp-build). not sure why they fixed node-gyp-buildbut notprebuildify. it looks like prebuildifyshould be fixed, but when i tried building withnode-gyp-build@4.8.1(which has the fix)prebuildify` failed. i didn't look further than that.

node-gyp itself might fail after that but i'll worry about that when we can get that far.

[bmacnaughton]

i take that back - it looks like i had something in my environment messed up.

installing node-gyp-build@4.8.1 seems to work. i did update node-gyp to version 10.1.0 though and haven't tested without it.

so

1. update node-gyp-build@4.8.1

if that fails then
2) update node-gyp@10.1.0 (globally)

[bmacnaughton]

one point - windows nvm can't download 22.1.0, so maybe it's missing? if i change this locally to -t 22.0.0 it works.

i was hasty on this. more to come.

[bmacnaughton]

more to come. i've been way off on this. i am now to where i reproduced the problem.

[bmacnaughton]

my (now slightly informed) thinking is that it's related to 1) the new version of v8 and/or 2) the new version of v8 requiring c++20.

will keep looking; sorry for the bad replies earlier.

[bmacnaughton]

ok, problem is a conflict with a windows definition.

you can temporarily fix by changing lines in node's v8-function-callback.h to wrap std::numeric_limits<uint16_t>::min in parens as follows:

template <typename T>
void ReturnValue<T>::Set(uint16_t i) {
  static_assert(std::is_base_of<T, Integer>::value, "type check");
  using I = internal::Internals;
  static_assert(I::IsValidSmi((std::numeric_limits<uint16_t>::min)()));
  static_assert(I::IsValidSmi((std::numeric_limits<uint16_t>::max)()));
  SetInternal(I::IntToSmi(i));
}

i'll submit a bug to node - i think it's their problem. they'll tell me if not.

[bmacnaughton]

nodejs/node#52895

[bmacnaughton]

was already fixed and will be in next node release.

[bizob2828]

What a journey. Thanks for sharing that discovery @bmacnaughton. I'll just wait for the next node 22 version to be released and update this PR

[bmacnaughton]

What a journey. Thanks for sharing that discovery @bmacnaughton. I'll just wait for the next node 22 version to be released and update this PR

thanks. once i got past my false starts it was pretty straightforward.

[bizob2828]

since y'all dropped 12 and 14 upgrading npm is no longer needed as 16 ships with 8

[bizob2828]

@bmacnaughton Node 22.2.0 came out so I updated this Pr and now everything is ✅

[bmacnaughton]

sorry for the delay - been ooo.

agent v5 no longer supports this but it's used in v4 too and that still gets fixes when required.

could you add node 12 and 14 back in?

[bizob2828]

sorry for the delay - been ooo.

agent v5 no longer supports this but it's used in v4 too and that still gets fixes when required.

could you add node 12 and 14 back in?

@bmacnaughton I didn't remove 12 and 14. That landed here and has since been released

[bmacnaughton]

my bad, i missed that while ooo. there was a decision to lock v4 to old versions.

[bmacnaughton]

thanks for the changes, Bob.

[bizob2828]

@bmacnaughton I cannot merge, but can you do that and cut a new release please?

[bmacnaughton]

@bizob2828 i have been ooo again and just merged this. @jcolekaplan noticed that you were missing -t before the 22.2.0. is there a reason you don't want that? i think that -t should be present, but maybe i'm missing something.

[bizob2828]

ah crap. i can fix that

[bizob2828]

@bmacnaughton FWIW this works but I PRd to avoid confusion