windows-registry-node icon indicating copy to clipboard operation
windows-registry-node copied to clipboard

WIP / requesting feedback: Node 10 support.

Open simonbuchan opened this issue 6 years ago • 4 comments

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 #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!

simonbuchan avatar Sep 25 '18 04:09 simonbuchan

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.

RupW avatar Sep 25 '18 06:09 RupW

@ritazh any plans on merging this?

zbynek avatar Dec 06 '19 15:12 zbynek

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

baparham avatar Jan 13 '20 10:01 baparham

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.

simonbuchan avatar Jan 14 '20 08:01 simonbuchan