-
Notifications
You must be signed in to change notification settings - Fork 53
Fix for MSVC 2022, node 22, on Windows to use more specific declarations+casts. #280
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
Conversation
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.
LGTM - getting this released would be super useful! @sergiou87 this library is currently unusable in Node v22 (i.e. current LTS Node) without this change.
src/main.cc
Outdated
@@ -42,7 +42,7 @@ LPWSTR utf8ToWideChar(std::string utf8) { | |||
return result; | |||
} | |||
|
|||
Napi::Object CreateEntry(Napi::Env& env, LPWSTR name, LPWSTR type, LPWSTR data, DWORD dataLengthBytes) | |||
Napi::Object CreateEntry(const Napi::Env& env, LPWSTR name, LPWSTR type, LPWSTR data, DWORD dataLengthBytes) |
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.
Could we add const
to the type
parameter so we can avoid the const_cast
uses?
Napi::Object CreateEntry(const Napi::Env& env, LPWSTR name, LPWSTR type, LPWSTR data, DWORD dataLengthBytes) | |
Napi::Object CreateEntry(const Napi::Env& env, LPWSTR name, const LPWSTR type, LPWSTR data, DWORD dataLengthBytes) |
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.
Good idea, I adjusted. This isn't yet updating node-gyp in package.json, but needs at least node-gyp@9 to recognize MS VisualStudio 2022, e.g. [email protected] , or the latest now is 11.2.0.
4e33838
to
756d73a
Compare
756d73a
to
256baa7
Compare
I've updated to remove an unwanted change to package.json and setting node-gyp ^11.2.0 in devDependencies. |
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.
Thank you! It seems you will need to bump node
too:
error [email protected]: The engine "node" is incompatible with this module. Expected version "^18.17.0 || >=20.5.0". Got "12.22.12"
I think this is the line you need to change: registry-js/.github/workflows/ci.yml Line 30 in 8183fca
|
Thanks, I increased the ci.yml to node-version: 18 . |
06c2047
to
3c5721b
Compare
Sorry for the missing adjustment to package-lock.json, I've updated that with |
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.
Thank you for this!
With node@22 and MS VisualStudio 2022 BuildTools on Windows 10, the registry-js package was failing to build via node-gyp.
I had to upgrade to at least node-gyp@9 (latest node-gyp@11) for it to recognize VisualStudio 2022, and then there were build errors with main.cc that after some investigation appeared to need stricter declaration/casting. I've attempted a reasonable fix based on the errors although I'm not a C/C++ expert. This now npm installs, builds, and
npm run test
is successful.