-
-
Notifications
You must be signed in to change notification settings - Fork 525
Adding new Nan::TryEncode() wrapper for node::TryEncode()
#1005
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
Conversation
|
I took a quick check and it looks alright, this is fine to merge as-is. However, I had a thought. Could not the old functions relying on the deprecated API be rewritten to use the new |
|
I think only Node version 24+ can use it but yes I could rewrite it to use |
|
I was referring to a comment in the node.js commit that introduced the new
MaybeEncode. Somewhere there it said that the old Encode did a
ToLocalChecked at some point which would then cause an error.
Conditioning Nan::Encode on NODE_VERSION >= 24, one that forwards to
Nan::MaybeEncode and another with the current one sounds perfect.
…On Tue, Dec 2, 2025, 18:21 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1005)
<#1005 (comment)>
I think only Node version 24+ can use it but yes I could rewrite it to use
TryEncode() only for those versions. Not sure what you mean by suggesting
to use ToLocalChecked() since TryEncode() returns the same type of
variable unless I misunderstand what you trying to do.
—
Reply to this email directly, view it on GitHub
<#1005 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKKZIRE4BPD7HM4FIXT37W4B7AVCNFSM6AAAAACNZ6ELC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMMBSHA4TENZYGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I made a mistake in code |
|
Question - since it's a direct call to |
|
Nan::MaybeLocal is just an alias for v8::MaybeLocal in all versions that
have the Maybe-stuff.
https://github.com/nodejs/nan/blob/4f2ac29d7f08a8f10b7dfb7f9e7f1b05932970dc/nan_maybe_43_inl.h#L13
I'd go with Nan::MaybeLocal
…On Tue, Dec 2, 2025, 19:04 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1005)
<#1005 (comment)>
Question - since it's a direct call to node::TryEncode() should it return
v8::MaybeLocal<> instead of Nan::MaybeLocal<>
—
Reply to this email directly, view it on GitHub
<#1005 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKJV6FC6K34WVNH3UL337XBCJAVCNFSM6AAAAACNZ6ELC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMMBTGA4TKMRVGQ>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Have another question and also want to make clear that C++ is not really a language I am too familiar with. I was not able to reuse or call #if NODE_MAJOR_VERSION >= 24
inline MaybeLocal<v8::Value> TryEncode(
const char *buf, size_t len, enum Encoding encoding = BINARY) {
v8::Isolate* isolate = v8::Isolate::GetCurrent();
node::encoding node_enc = static_cast<node::encoding>(encoding);
if (encoding == UCS2) {
return node::TryEncode(
isolate
, reinterpret_cast<const uint16_t *>(buf)
, len / 2);
} else {
return node::TryEncode(
isolate
, reinterpret_cast<const char *>(buf)
, len
, node_enc);
}
}
NAN_DEPRECATED inline v8::Local<v8::Value> Encode(
const void *buf, size_t len, enum Encoding encoding = BINARY){
v8::Isolate* isolate = v8::Isolate::GetCurrent();
node::encoding node_enc = static_cast<node::encoding>(encoding);
if (encoding == UCS2) {
return node::TryEncode(
isolate
, reinterpret_cast<const uint16_t *>(buf)
, len / 2).ToLocalChecked();
} else {
return node::TryEncode(
isolate
, reinterpret_cast<const char *>(buf)
, len
, node_enc).ToLocalChecked();
}
}
#else
...
#endifWhat am I missing? |
|
Your |
|
I did not do the necessary ifdeffing, so it is obviously not right, but it compiles. |
|
I do not know how I missed that and actually no idea where void came from in the first place. I copy/pasted Thank you! |
|
I was wondering about the origin of the void pointer myself. The old node API also uses char pointers in the latest version. Either it used to be a void pointer a long time ago, or I changed it to a void pointer, because the data is not necessarily a char array at all (e.g. utf-16) making a char pointer semantically wrong. |
|
In my code I marked |
Your second assumption is correct, tests do not build with a non void parameter. |
|
Yes and no. I care about the NAN API being consistent across versions.
Since versions prior to Node 24 cannot achieve identical behavior to later
versions, due to node::TryEncode not being available and the aforementioned
ToLocalChecked in node::Encode not making it possible to wrap its output in
case the option is empty, the only way to have it work everywhere is by
using Nan::Encode.
So, while I would like to deprecate Nan::Encode in favor of Nan::TryEncode,
I do not see a way of doing it right without too much effort for a niche
problem (replicating the entirety of node::TryEncode). Everyone was fine
with the old variant for some ten years. Perhaps a note in the
documentation about whether to use one or the other is sufficient.
…On Tue, Dec 2, 2025, 21:25 agracio ***@***.***> wrote:
*agracio* left a comment (nodejs/nan#1005)
<#1005 (comment)>
In my code I marked Encode() as NAN_DEPRECATED for Node >=24 to make
users aware of deprecation, is it not something you want to implement?
—
Reply to this email directly, view it on GitHub
<#1005 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7JXKPVBCYKFRV4X5XRESL37XRTHAVCNFSM6AAAAACNZ6ELC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMMBTGYZTCMZWHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
I can do it either way, the only issue I see is that if it is not marked as deprecated and Node native warning is suppressed many users will not even be aware of the change and that they might want to look at it. Although flooding some users with warning is also not ideal. |
…oved deprecation for `Encode()` in Node >= 24
|
Changes complete |
|
Wonderful. Thank you very much for your effort. I will push a new release. |
Nan::EncodeandNan:TryEncode()changesNan:TryEncode()method for Node version 24 and higherNan::Encodefor Node version 24 and higherstring_bytes.mdto reflect changes and rebuilt READMEMiscellaneous
NODE_MODULE_VERSIONdefinition for all Node version to maintain consistencymacos-13tomacos-15-intelto avoid deprecationAppVeyor tests for older versions of nan that are currently disabled in this repo (https://ci.appveyor.com/project/agracio/nan)