chore: overload deprecated AccessorSignatures

[VerteDinde]

AccessorSignatures was removed in https://chromium-review.googlesource.com/c/v8/v8/+/3654096 . This PR overloads the SetAccessor method, removing the default parameters, and also includes the original method with the signature param removed. This should allow older versions of Node to use signature, but fixes nan for newer versions of Node and Electron

Supersedes: #941 Fixes: https://github.com/nodejs/nan/issues/942

@kkoopa This PR attempts to address the feedback left here: https://github.com/nodejs/nan/pull/941#issuecomment-1162385143 If this doesn't encompass what you were thinking, let me know, happy to make adjustments!

[alexciarlillo]

I am trying this out since the branch for #941 (which was working) no longer exists. I am wondering where the choice of node module version 18 comes from. I am on node 16.15.0 and trying to compile a module against electron 20 (also 16.15.0) and the version check here doesn't work for me. If I modify it to NODE_16_0_MODULE_VERSION it works as expected.

[MRayermannMSFT]

@alexciarlillo what errors did you get when trying out this branch? I got errors like

error C2665: 'v8::ObjectTemplate::SetAccessor': none of the 2 overloads could convert all the argument types

Which like you, I did not get with the changes from the previous PR.

[alexciarlillo]

I'm not sure if I got that error but IIRC I was also still getting

error: ‘AccessorSignature’ is not a member of ‘v8’

Sorry I should have kept better track and posted more details. I have just been in dependency hell for like a week trying to do an electron upgrade and my brain was a bit fried. But I can confirm this fix doesn't work for me on Node 16.15.x where the previous one did.

[sebastianrath]

Same here, it reports this one as still being used:

https://github.com/VerteDinde/nan/blob/1719071c1b74e569f7e6f57c589fc9e5020a3f6e/nan.h#L2546-L2556

    npm ERR! In file included from ../../nan/nan.h:180:
    npm ERR! ../../nan/nan_callbacks.h:55:23: error: no member named 'AccessorSignature' in namespace 'v8'
    npm ERR! typedef v8::Local<v8::AccessorSignature> Sig;
    npm ERR!                   ~~~~^
    npm ERR! In file included from ../src/functions.cpp:20:
    npm ERR! In file included from ../src/functions.h:4:
    npm ERR! ../../nan/nan.h:2546:8: error: no matching member function for call to 'SetAccessor'
    npm ERR!   tpl->SetAccessor(

Built on macOS for: Local node: v16.15.0 Electron v19.0.15

[kkoopa]

Yes, something like this was what I had in mind, API-wise.

[kkoopa]

As for the AccessorSignature, having a dummy one as Data should work.

@@ -53,7 +53,9 @@ typedef void(*IndexQueryCallback)(

 namespace imp {
 #if (NODE_MODULE_VERSION < NODE_18_0_MODULE_VERSION)
-NAN_DEPRECATED typedef v8::Local<v8::AccessorSignature> Sig;
+typedef v8::Local<v8::AccessorSignature> Sig;
+#else
+typedef v8::Local<v8::Data> Sig;
 #endif

 static const int kDataIndex =                    0;

[miniak]

Can we expedite merging this fix and releasing a new version? We cannot build our node modules using NaN for Electron 20, which has been out for over a month

[segmentedcontrol]

@kkoopa

[VerteDinde]

@kkoopa Done, thanks! Added the dummy sig as suggested.

[MRayermannMSFT]

@VerteDinde this branch appears to work well for me now. :)

[michalzaq12]

[miniak]

FYI Electron 21 was released in the meantime

[devinbinnie]

Any idea when this will get merged? Trying to upgrade my app to Electron v20 and am currently blocked on this :(

[alejandroclaro]

Any update?

[alejandroclaro]

In case someone is looking for a temporal workaround: https://github.com/electron/electron/issues/35193

[miniak]

Looks like it's faster for us to just replace the usage of NaN with pure V8 calls than to wait for the fix :(

[RaisinTen]

I might be missing something but why replicate SetAccessor() in its entirety when the only thing that's different is the disappearance of the signature argument?

@bnoordhuis I think that's because of @kkoopa's review comment in the previous PR - https://github.com/nodejs/nan/pull/941#issuecomment-1162385143.

[kkoopa]

Sorry, completely forgot about this during vacation. I'll go through it now.

[kkoopa]

Yes, this looks like what I had in mind and it seems to pass the tests. I need to add some more entries to the test matrix and do some maintenance before I can push a new release. Thank you all.

[kkoopa]

New release published. Hopefully this resolves all problems.

[segmentedcontrol]

all hail benjamin!

[MRayermannMSFT]