-
-
Notifications
You must be signed in to change notification settings - Fork 522
Adding WriteUtf8V2() and SetPrototypeV2() methods to support v8 14.3 #1004
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
CI on Node.js 17.x was not failing in my branch: https://github.com/agracio/nan/actions/runs/18606064924. |
Thank you. This looks fine to me, still need to look at the the WriteUtf8 bit in some more detail, since it is fairly complicated, but presumably all should be good. |
Found another issue with v8 14.3, this one is trickier to address. v8 12.8 - 14.2 enum V8_DEPRECATE_SOON(
"This enum is no longer used and will be removed in V8 12.9.")
AccessControl {
DEFAULT V8_ENUM_DEPRECATE_SOON("not used") = 0,
}; v8 14.3 enum V8_DEPRECATED(
"This enum is no longer used and will be removed in V8 14.3.")
AccessControl {
DEFAULT V8_ENUM_DEPRECATED("not used") = 0,
}; I am not sure how it manages to have a deprecation notice in v8 14.3 while being marked for deprecation in the same version. However don't think it is worth taking any chances. Context: initially I had another issue masking this warning and only saw it yesterday, v8 14.3 modules currently compile but with a warning. This enum is used in nan.h |
@kkoopa apologies for direct ping, but could you confirm that nan is currently targeting Node.js version 8 and above and no longer officially support earlier versions in latest release? There are some inconsistences in Unfortunately I do not think it would be possible to address Example of code inconsistency // ... SetAccessor() code
#if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION > 12 \
|| (V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) \
&& V8_MINOR_VERSION >= 5))
tpl->SetNativeDataProperty(
#else
tpl->SetAccessor(
#endif
name
, getter_
, setter_
, obj
#if !defined(V8_MAJOR_VERSION) || V8_MAJOR_VERSION < 12
, settings
#endif
, attribute
); |
Multiple definitions but transparent to the user of the API? While it would
be extra work, it would atill work nicely in that case.
It is still all old versions being supported, but that could change if
strictly necessary. I do not remember when Node 6.0 was released, but it
was probably around 10 years ago.
…On Wed, Oct 22, 2025, 17:50 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1004)
<#1004 (comment)>
@kkoopa <https://github.com/kkoopa> apologies for direct ping, but could
you confirm that nan is currently targeting Node.js version 8 and above and
no longer officially support earlier versions in latest release? There are
some inconsistences in SetAccessor() methods and one of them (only one
for some reason) has a Node.js version dependency >=6 that I believe is no
longer needed.
Unfortunately I do not think it would be possible to address AccessControl
enum deprecation issue without having multiple SetAccessor() methods
depending on v8 version.
—
Reply to this email directly, view it on GitHub
<#1004 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKKYVBI5J3VTG4AZOJ33Y6KSXAVCNFSM6AAAAACJQWTRKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZSG43DOMJRHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
I would not say removing older versions is strictly necessary just makes the code considerably more convoluted. One of the changes I already pushed in this PR did make an assumption that 0.x versions are no longer supported. |
I am out and about on my phone right now, so can't dig in much, only
speculate. What does the git blame history say on those lines? My first
guess is that it could be a slightly miss-hedged fix supposed to also cover
some then current version of Electron.
…On Wed, Oct 22, 2025, 18:02 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1004)
<#1004 (comment)>
I would not say removing older versions is strictly necessary just makes
the code considerably more convoluted. One of the changes I already pushed
in this PR did make an assumption that 0.x versions are no longer supported.
Node.js version 6 check is strange since it only done in one of 3
SetAccessor() methods.
—
Reply to this email directly, view it on GitHub
<#1004 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKMZQEHQ3SVZKKZY5CT3Y6MA5AVCNFSM6AAAAACJQWTRKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZSHA2DCOBSGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
For this code it is part of PR #977. // void SetAccessor()
#if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION > 12 \
|| (V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) \
&& V8_MINOR_VERSION >= 5))
tpl->SetNativeDataProperty(
#else
tpl->SetAccessor(
#endif
name
, getter_
, setter_
, obj
#if !defined(V8_MAJOR_VERSION) || V8_MAJOR_VERSION < 12
, settings
#endif
, attribute
); However as part of the same PR the 3rd // bool SetAccessor()
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)
#if defined(V8_MAJOR_VERSION) && \
(V8_MAJOR_VERSION > 12 || \
(V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) && \
V8_MINOR_VERSION >= 5))
return obj->SetNativeDataProperty(
GetCurrentContext()
, name
, getter_
, setter_
, dataobj
, attribute).FromMaybe(false);
#else
return obj->SetAccessor(
GetCurrentContext()
, name
, getter_
, setter_
, dataobj
, settings
, attribute).FromMaybe(false);
#endif
#else
return obj->SetAccessor(
name
, getter_
, setter_
, dataobj
, settings
, attribute);
#endif For v8 14.3 and above Current signature of one of the methods: inline void SetAccessor(
v8::Local<v8::ObjectTemplate> tpl
, v8::Local<v8::String> name
, GetterCallback getter
, SetterCallback setter = 0
, v8::Local<v8::Value> data = v8::Local<v8::Value>()
, v8::AccessControl settings = v8::DEFAULT // v8::AccessControl has to be removed
, v8::PropertyAttribute attribute = v8::None)
|
Oh look, I myself am the culprit... Don't remember why it was done that
way, best guess is just repeating a local ifdef pattern. As for remocing
the defaulted argument, yes, rhe way I have solved that in the past is by
adding a bunch of overloads to redo the effects of dedault aeguments. I
blame part on C++ default arguments which are unfortunately broken beyond
repair and should not be used in any well-designed API, for these and
similar reasons.
…On Wed, Oct 22, 2025, 18:22 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1004)
<#1004 (comment)>
For this part of code it is part of PR #977
<#977>.
// void SetAccessor()
#if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION > 12 \
|| (V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) \
&& V8_MINOR_VERSION >= 5))
tpl->SetNativeDataProperty(
#else
tpl->SetAccessor(
#endif
name
, getter_
, setter_
, obj
#if !defined(V8_MAJOR_VERSION) || V8_MAJOR_VERSION < 12
, settings
#endif
, attribute
);
However as part of the same PR the 3rd bool SetAccessor() has a different
logic that appears to be more consistent.
// bool SetAccessor()
#if (NODE_MODULE_VERSION >= NODE_6_0_MODULE_VERSION)
#if defined(V8_MAJOR_VERSION) && \
(V8_MAJOR_VERSION > 12 || \
(V8_MAJOR_VERSION == 12 && defined(V8_MINOR_VERSION) && \
V8_MINOR_VERSION >= 5))
return obj->SetNativeDataProperty(
GetCurrentContext()
, name
, getter_
, setter_
, dataobj
, attribute).FromMaybe(false);
#else
return obj->SetAccessor(
GetCurrentContext()
, name
, getter_
, setter_
, dataobj
, settings
, attribute).FromMaybe(false);
#endif
#else
return obj->SetAccessor(
name
, getter_
, setter_
, dataobj
, settings
, attribute);
#endif
For v8 14.3 and above SetAccessor() method signature will change because v8::AccessControl
settings is deprecated and has to be removed, I am not sure how this is
going to affect any modules that are rely on nan, will check my own modules.
Current signature of one of the methods:
inline void SetAccessor(
v8::Local<v8::ObjectTemplate> tpl
, v8::Local<v8::String> name
, GetterCallback getter
, SetterCallback setter = 0
, v8::Local<v8::Value> data = v8::Local<v8::Value>()
, v8::AccessControl settings = v8::DEFAULT // v8::AccessControl has to be removed
, v8::PropertyAttribute attribute = v8::None)
—
Reply to this email directly, view it on GitHub
<#1004 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKJYJQDY7WFHLHC4TJ33Y6OKLAVCNFSM6AAAAACJQWTRKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZSHE4DIOJYHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
So should I proceed with changes to |
Backwards compatible how? Having to change existing code is mostly fine
(although the impact ahould be minimized) as long as it then builds
correctly for all supported versions. If this cannot be done, I guess
proceeding with the largest possible version coverage is the second best
thing.
…On Wed, Oct 22, 2025, 18:42 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1004)
<#1004 (comment)>
So should I proceed with changes to SetAccessor() signature for v8 >=14.3?
I do not see any possibility to keep method signature backward compatible.
—
Reply to this email directly, view it on GitHub
<#1004 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKPX5J4FZVVGVKARAJL3Y6QYDAVCNFSM6AAAAACJQWTRKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZTGA3TKNJYGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Sorry maybe I have not explained the change properly. Current nan nan inline void SetAccessor(
v8::Local<v8::ObjectTemplate> tpl
, v8::Local<v8::String> name
, GetterCallback getter
, SetterCallback setter = 0
, v8::Local<v8::Value> data = v8::Local<v8::Value>()
, v8::AccessControl settings = v8::DEFAULT // v8::AccessControl has to be removed
, v8::PropertyAttribute attribute = v8::None) |
Is v8::AccessControl is still a plain enum? That makes it an int, so the
enum itself could be replicated as enum Nan::AccessControl if needed. As
for the default arguments, can't they be dealt with by adding a bunch of
overloads with varying number of arguments instead?
In general,
int foo(int bar, int baz=0);
can be replaced with
int foo(int bar, int baz);
int foo(int bar) {
foo(bar, 0);
}
…On Wed, Oct 22, 2025, 19:01 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1004)
<#1004 (comment)>
Sorry maybe I have not explained the change properly.
Current *nan* SetAccessor() method parameters include parameter v8::AccessControl
settings = v8::DEFAULT. When this enum is removed from v8 the parameter
can no longer be part of the method signature.
Removing this parameter will change the signature of this method unless I
am missing something and there is some kind of way to preserve it.
My proposal would be to create new SetAccessor() methods with new
signatures for v8 >= 14.3 while keeping the old methods for older versions
of v8.
nan SetAccessor() current signature
inline void SetAccessor(
v8::Local<v8::ObjectTemplate> tpl
, v8::Local<v8::String> name
, GetterCallback getter
, SetterCallback setter = 0
, v8::Local<v8::Value> data = v8::Local<v8::Value>()
, v8::AccessControl settings = v8::DEFAULT // v8::AccessControl has to be removed
, v8::PropertyAttribute attribute = v8::None)
—
Reply to this email directly, view it on GitHub
<#1004 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKJJ4H7PM5VME5FMBEL3Y6S4BAVCNFSM6AAAAACJQWTRKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMZTGE2DENZUHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Not sure that I am 100% following you but do you mean something along these lines? #if defined(V8_MAJOR_VERSION) && (V8_MAJOR_VERSION > 14 \
|| (V8_MAJOR_VERSION == 14 && defined(V8_MINOR_VERSION) \
&& V8_MINOR_VERSION >= 3))
// v8::AccessControl settings = v8::DEFAULT parameter is not defined in signature
inline void SetAccessor(
v8::Local<v8::ObjectTemplate> tpl
, v8::Local<v8::String> name
, GetterCallback getter
, SetterCallback setter = 0
, v8::Local<v8::Value> data = v8::Local<v8::Value>()
, v8::PropertyAttribute attribute = v8::None) {
SetAccessor(
tpl,
name,
getter,
setter,
data,
0, // v8::AccessControl settings = v8::DEFAULT - v8::DEFAULT equals 0 according to the docs,
attribute);
}
#else
inline void SetAccessor(
v8::Local<v8::ObjectTemplate> tpl
, v8::Local<v8::String> name
, GetterCallback getter
, SetterCallback setter = 0
, v8::Local<v8::Value> data = v8::Local<v8::Value>()
, v8::AccessControl settings = v8::DEFAULT
, v8::PropertyAttribute attribute = v8::None) {
// original code
}
#endif |
Nvm this does not work. |
Reason for this PR
Adding compatibility with v8 version 14.3
WriteUtf8V2() method
WriteUtf8V2()
method was introduced in v8 13.4 and has a slightly different signature compared toWriteUtf8()
.WriteUtf8()
method and associated enum were removed in v8 14.3.SetPrototypeV2() method
SetPrototypeV2()
method was introduced in v8 14.0 with same signature asSetPrototype()
.SetPrototype()
method was removed in v8 14.3.CI Changes
CI Results