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

WIP / requesting feedback: Node 10 support. #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

WIP / requesting feedback: Node 10 support. #62

wants to merge 2 commits into from

Conversation

simonbuchan
Copy link

@simonbuchan simonbuchan commented Sep 25, 2018

Sitting on #59, so look at that first.

This code should work, but I'd like some feedback on the changes first. In particular, how the utils.associateExeForFile() test should work without mocks (switch to registering in HKCU\Classes?), and any preferences on how errors should work / look.

TODO:

  • ffi-napi has ref-napi and ref-struct-di as deps, so probably should also switch to those, although it seems to work currently?
  • Find a way to test associateExeForFile() without mocks / restore TEST_MOCKS_ON.
  • Fallback for Buffer pre-node 5.10?
  • Non placeholder-ey keyError() - currently pretty weak formatting.

Running on node 10 has a couple of problems:

  • ffi package fails to build against recent version of node, with several errors like <snip>\ffi.cc(111): error C2039: 'ForceSet': is not a member of 'v8::Object' - it doesn't seem to be maintained. Simply replace with ffi-napi, which forks it and uses NAPI to preserve compat.
  • new Buffer() is finally deprecated, and will now print warnings like (node:5056) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.. Replaced all instances with Buffer.alloc(size, fill?) or Buffer.from(content, encoding?) added in node 5.10.

While getting tests running ran into ACCESS_DENIED errors, so I threw in some fixes for tests:

  • As mentioned above, forking Fix key and registry tests to work on Windows 10 #59 so tests run further on Win10.
  • Fixed running tests with mock (missing predefined keys, delete key mock) and always mock (ignore TEST_MOCKS_ON), as utils.associateExeForFile() will die without it, as it writes to HKCU.
  • While running tests, found the code was throwing strings, which is terrible (you don't get error stacks). Replaced them with throw new Error(message), a very placeholder keyError(message, result, ...args) that will create these, and updated lots of error messages that were claiming "Failed to open key" when they were doing no such thing.
  • It was a bit annoying to be asked to npm i -g grunt-cli in 2018, so I added it as a local dep, which works fine!

RupW and others added 2 commits June 15, 2018 19:30
- Open keys in HKCR as read-only, not all access
- Create a new key HKCU\Software\windows-registry-node for read/write
  tests.

Note that this key is not cleaned up on test completion, in case the
delete function deletes something it shouldn't!

The file association test still attempts to write to HKCR and will fail.
@RupW
Copy link

RupW commented Sep 25, 2018

As you've probably discovered the CI is configured for node 4 in appveyor.yml, which means the new Buffer forms won't work. I think it's worth updating that to something newer - hopefully one of the maintainers can comment on that, i.e. what version of node we should support for the future?

Node.JS 4.x was EOL-ed back in April, so we have a reasonable case for an upgrade: https://github.com/nodejs/Release#end-of-life-releases

(I'm also not sure exactly what version of Windows - or Wine? - the CI server is running, since it passes the unit tests without access errors.)

The unreleased ffi master branch did work for me on node 9 at least; ffi-napi may well be the way to go anyway but I don't know enough to comment. Ditto thanks for fixing the throws: I didn't know enough Node.js to do that properly myself.

@zbynek
Copy link

zbynek commented Dec 6, 2019

@ritazh any plans on merging this?

@baparham
Copy link

Is there insight into where this proposal is going? Do we have any moderator support for this project currently? @ritazh @sedouard ?

@simonbuchan
Copy link
Author

If you're looking for any registry package, I ended up writing my own from scratch: https://github.com/simonbuchan/native-reg
but keep in mind it's a different API, e.g. C-style rather than Key objects.

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.

4 participants