fix: remove AllCan Read/Write

[MarshallOfSound]

Refs https://chromium-review.googlesource.com/c/v8/v8/+/5006387

Fixes native module compilation against node 21 / incoming v8 changes

[kkoopa]

Thank you. I am concerned about the signature changes. It seems like it should be possible to paper over the differences without breaking the NAN API. Assuming that v8::AccessControl is just a straight enum, it should be replaceable by an int. SetAccessor could be deprecated (again...) and a new one introduced without the settings parameter, shoving default for old versions. Now, due to the bad choice of using default arguments, C++ overload resolution becomes a mess I no longer remember. Might need to make a bunch of overloads for varying number of arguments instead, which is how it should have been written in the first place, since default arguments are bad design and should not be used, but I digress.

On November 14, 2023 11:13:41 PM GMT+02:00, Samuel Attard @.***> wrote:

Refs https://chromium-review.googlesource.com/c/v8/v8/+/5006387

Fixes native module compilation against node 21 / incoming v8 changes You can view, comment on, or merge this pull request online at:

https://github.com/nodejs/nan/pull/963

-- Commit Summary --

• fix: remove AllCan Read/Write

-- File Changes --

M nan.h (19)

-- Patch Links --

https://github.com/nodejs/nan/pull/963.patch https://github.com/nodejs/nan/pull/963.diff

[MarshallOfSound]

@kkoopa I could move settings after attribute and default it correctly in all cases, that should avoid the signature being meaningfully different 🤔 Although annoying because it wouldn't match the argument order to v8 but not the end of the world if the nan helper is deprecated anyway

[kkoopa]

Yes, that might work. All I want is to avoid breaking NANs API directly while retaining the same observable behavior across versions.

I remember dealing with something similar in one of these default parameter functions previously. I ended up adding n overloads of increasing arity, where n was the number of default parameters, which happens to be the correct way of doing it in the first place. Default parameters in C++ are garbage.

On November 20, 2023 9:26:22 PM GMT+02:00, Samuel Attard @.***> wrote:

@kkoopa I could move settings after attribute and default it correctly in all cases, that should avoid the signature being meaningfully different 🤔 Although annoying because it wouldn't match the argument order to v8 but not the end of the world if the nan helper is deprecated anyway

[MarshallOfSound]

Turns out the actual enum hasn't been removed, so the nan signature can be the same we just need to safely ifdef the impl.

Ref: https://source.chromium.org/chromium/chromium/src/+/main:v8/include/v8-object.h;l=173;bpv=1;bpt=1?q=v8::AccessControl&ss=chromium

Should be safe now

[kkoopa]

Wonderful. That makes it a lot easier. Now, I would still like to have Nan::SetAccessor as it is in this PR deprecated and a new one added which does not have the settings parameter. Then, for old versions, it would set settings to Default when forwarding. This serves to maintain the identical behavior across versions: if one can't specify AccessControl, none shall do so. It also signals that a change has happened.