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

Update nan to latest version. #56

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

travispaul
Copy link

Fixes #55

@stoneLeaf
Copy link

Thank you @travispaul.
@stephenwvickers Please, could you merge that one?

@stoneLeaf
Copy link

My bad, I actually missed a compile error even with the fix:

npm error C:\xxxxx\node_modules\os-service\src\service.cc(90,34): error C2440: 'initializing': cannot convert from 'const char [1]' to 'LPSTR' [C:\xxxxx\node_modules\os-service\build\service.vcxproj]
npm error       C:\xxxxx\node_modules\os-service\src\service.cc(90,34):
npm error       Conversion from string literal loses const qualifier (see /Zc:strictStrings)
npm error gyp info it worked if it ends with ok
npm error gyp info using [email protected]
npm error gyp info using [email protected] | win32 | x64
npm error gyp info find Python using Python version 3.12.4 found at "C:\Python312\python.exe"
npm error gyp info find VS using VS2022 (17.11.35222.181) found at:
npm error gyp info find VS "C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools"

@travispaul
Copy link
Author

@stoneLeaf I've encountered that with v22 recently too (didn't see that error on v20 though), I have a fix here: travispaul@6445d7d

@stoneLeaf
Copy link

I can confirm it's working under Node 22 with your fix @travispaul, thanks a lot!
I might switch to another library like github.com/winsw/winsw if this one is not maintained anymore though.

@travispaul
Copy link
Author

I might switch to another library like github.com/winsw/winsw if this one is not maintained anymore though.

Yep, makes sense.

I'm planning to fork this library for work at some point in the future (unless activity picks back up), but its not highest priority at the moment. If/when we do fork it, we'll also remove all the non-windows features in our fork (we'll handle *nix daemons through other means) and keep it working with latest and LTS Node versions.

@stoneLeaf
Copy link

stoneLeaf commented Oct 31, 2024

I'm planning to fork this library for work at some point in the future (unless activity picks back up), but its not highest priority at the moment. If/when we do fork it, we'll also remove all the non-windows features in our fork (we'll handle *nix daemons through other means) and keep it working with latest and LTS Node versions.

I only need it for Windows too so I'll keep an eye out for your fork if it happens and you keep it public.
Thanks again mate!

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

Successfully merging this pull request may close these issues.

Support Node v20
2 participants