search

nodejs / nan

Native Abstractions for Node.js
MIT License
3.28k stars 504 forks source link

chore: remove deprecated AccessorSignatures #941

Closed jkleinsc closed 2 years ago

jkleinsc commented 2 years ago

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

Co-Authored-By: Shelley Vohr shelley.vohr@gmail.com

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

kkoopa commented 2 years ago

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.

jkleinsc commented 2 years ago

Closing in favor of #943

jstark518 commented 2 years ago

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