bcoin-org / bcrypto

JS crypto library
Other
99 stars 41 forks source link

v8/node deprecated APIs and how to handle them #7

Closed chjj closed 5 years ago

chjj commented 6 years ago

Putting this here to document for my own sanity.

V8 removed a bunch of calls and the node.js devs have decided to maintain them for the time being, but have marked them as deprecated. We need to switch away from them.

Deprecated APIs:

Example migration:

Originally:

int32_t arg = info[0]->Int32Value();

Now must be:

int32_t arg = info[0]->Int32Value(Nan::GetCurrentContext()).FromJust();

Or:

int32_t arg = info[0]->Int32Value(Nan::GetCurrentContext()).ToChecked();

Or:

int32_t arg = info[0]->ToInt32(Nan::GetCurrentContext()).ToLocalChecked()->Value();

Int32Value returns Maybe<uint32_t>.

ToInt32 returns MaybeLocal<Int32>.

Note that ToChecked() is an alias for FromJust().

ToString() was also deprecated, but nan.h should be handling this. Not us, but for completeness, heres the migration.

From:

v8::Local<v8::String> str = info[0]->ToString();

To:

v8::Local<v8::String> str = info[0]->ToString(Nan::GetCurrentContext()).FromMaybe(v8::Local<v8::String>());

Note that anything that returns MaybeLocal instead of Maybe should have an IsEmpty() check.

Example (from the style guide below):

v8::MaybeLocal<v8::Value> maybe = fn->Call(...);

if (maybe.IsEmpty()) { // Exception occurs during function call
  // Do some clean up
  return; // Back to JavaScript and let it throw
}

v8::Local<v8::Value> result = maybe.ToLocalChecked();

References:

Timeline:

Style Guide (mentioning the changes):

I'm not sure how this will impact node versions prior to node v10. This maybe needs to be added as a part of nan? Not sure. Going to do more research.

Update:

Just discovered the magic of Nan::To (1, 2, 3).

We can do Nan::To<int32_t>(info[0]).FromJust().

Nan::To seems undocumented, but it seems to be the inverse of Nan::New.

chjj commented 6 years ago

It looks like nan has upgraded for this, but only for strings: https://github.com/nodejs/nan/commit/24a22c3b25eeeec2016c6ec239bdd6169e985447

Corresponding PR: https://github.com/nodejs/nan/pull/808

Nevermind, looks like it's still in the works: https://github.com/nodejs/nan/pull/811

chjj commented 5 years ago

Finally fixed with 3a3bb28f6d59d6c74108af89d4a9ef50e2e1429b

chjj commented 5 years ago

This issue seems to be getting a lot of attention from people in the community. Please note that Nan::To can be used in place of the nasty v8 syntax.

For example:

Nan::To<int32_t>(info[0]).FromJust()

Instead of:

info[0]->Int32Value(Nan::GetCurrentContext()).FromJust();
cocoche007 commented 4 years ago

Hi all,

In Node 8, I had:

v8::String::Utf8Value textV8(args[4]->ToString());
std::string           text(*textV8);

But in Node 12, with:

v8::Local<v8::String> textV8(args[4]->ToString(context).FromMaybe(v8::Local<v8::String>()));

I don't figure out how concert in std::string

Edit: For the moment, I found this solution but too complicated for me:

v8::Local<v8::String> textV8(args[4]->ToString(context).FromMaybe(v8::Local<v8::String>()));
Nan::Utf8String       textNan(textV8);
std::string           text(*textNan);
Rainmen-xia commented 4 years ago

marked