nan icon indicating copy to clipboard operation
nan copied to clipboard

chore: remove deprecated AccessorSignatures

Open jkleinsc opened this issue 3 years ago • 1 comments

This was removed in https://chromium-review.googlesource.com/c/v8/v8/+/3654096

Co-Authored-By: Shelley Vohr [email protected]

Fixes: https://github.com/nodejs/nan/issues/942

jkleinsc avatar Jun 21 '22 20:06 jkleinsc

While the signatures have never been widely in use, I am not too keen on outright changing the NAN API, since that would be a breaking change. Existing code should not break when compiled against an older Node version, making this problem somewhat trickier. The use of default arguments also makes it hard to create an overload without the signature parameter, but it should not be impossible, just requiring a whole load of overloads for the default arguments.

That would require creating one function with no default parameters (perhaps it would work to only make this one version with the signature, and a single other option retaining the other default parameters?), forwarding the others to this function, have a dummy implementation of imp::Sig for newer versions and do nothing with it, but retaining the existing behavior for older versions.

void SetAccessor(
    v8::Local<v8::ObjectTemplate> tpl
  , v8::Local<v8::String> name
  , GetterCallback getter
  , SetterCallback setter
  , v8::Local<v8::Value> data
  , v8::AccessControl settings
  , v8::PropertyAttribute attribute
  , imp::Sig signature);

The documentation needs updating either way. It can still be listed as having default parameters, since that would constitute an implemenation detail, but the one with a signaure argument marked deprecated and its replacement not having it, and finally having a deprecation warning on the function that gets run when giving a signature argument.

kkoopa avatar Jun 21 '22 21:06 kkoopa

Closing in favor of #943

jkleinsc avatar Sep 06 '22 18:09 jkleinsc

@jkleinsc possible to restore the remove_accessor_signature branch? It works and the branch in #943 does not.

jstark518 avatar Sep 12 '22 23:09 jstark518