Stop using of deprecated V8 API.

[sergz]

Background

We are using pretty a lot of deprecated V8 API, and in order to simplify our life, avoid unnecessary delays in such issues like #6063, and be able to experiment with new techniques like WebAssembly, we should update our code base to use up-to-date API.

What to change

The list of API

• [done] v8::TryCatch should accept v8::Isolate
• [done] v8::Isolate::IdleNotification is renamed to v8::Isolate::IdleNotitifcationDeadline
• [done] use maybe version of v8::Script::Compile, related #6448
• [done] use v8::Local instead of v8::Handle
• [done] use maybe version of v8::Message::GetLineNumber
• [question is below] use v8 functions which return maybe values, e.g.
  • v8::Value::IntegerValue
  • v8::FunctionTemplate::GetFunction
  • v8::String::NewFromUtf8
  • v8::Value::BooleanValue
  • v8::Object::Get(uint32_t)
  • v8::Object::GetOwnPropertyNames
  • v8::Function::Call
  • etc
• those maybe functions accept v8::Context, therefore we need to carefully check that we pass the correct context. Perhaps in the callbacks we should rather use v8::Isolate::GetCurrentContext instead of obtaining it from JsEngine. Additionally pay attention to JsContext which works with the context too.
• revisit ThrowExceptionInJS because failing throwing of an exception is not a good thing
• add actual tests simulating the state of the isolate when it starts to return empty values.
• to be continued ...

Here we need to decide regarding the behavior if there is no value in maybe value. The options are basically either to use an artificial default value, crash or propagate such resembling monadic approach to our API.

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - update usage of V8 API

[sergz]

I would like to get your opinion especially regarding the dealing with the maybevalues in our code.

In order to get compiler warnings and errors one can add

  'defines': [
    'V8_DEPRECATION_WARNINGS',
    'V8_IMMINENT_DEPRECATION_WARNINGS'
  ],

to 'target_defaults' in common.gypi and compile our code with prebuilt V8.

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - replace deprecated v8::Handle by v8::Local

[hfiguiere]

maybes are a good idea. As a Rust user I'm a big fan of Option<> (std::optional<> in C++ 17). While this often complicate code a bit, it definitely improve the code safety. It is like checking for nullptr.

I'm not sure if requiring C++17 for this is a good idea, but on the other hand we could have our own version of std::optional<> to implement this pattern.

[oleksandr]

I also think propagating to our API is probably best in the long term. Having default values could cause unintended consequences and crashing just doesn't seem right to me.

[sergz]

Replying to hfiguiere:

I'm not sure if requiring C++17 for this is a good idea, but on the other hand we could have our own version of std::optional<> to implement this pattern.

Yeah, taking into account that it's compiled by android NDK and possibly other not so fast toolsets, C++17 can be a challenge. Own implementation of std::optional is also causing rather more questions so far.

I would propose to start with own implementation of std::optional, let's call it Maybe with the explicit template specification only for JsValue so far, since it seems can be implemented in a relatively efficient way. Perhaps it will also cover most cases of the deprecated API.
JIC, here I mean that there will be a special implementation to work with internals of JsValue, not a generic one.

Additionally what do you think about map/flatmap/bind/get/ifPresent/orElse/etc functions (of course add them only when they are required) either as a mixin (perhaps via CRTP) or just template functions? E.g. it seems we are not using thousands of operations with them, so any overhead like lambdas should not be an issue but perhaps it reduces a huge arising boilerplate code.

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - pass v8::Isolate to more functions because old approach is deprecated

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - Use the maybe version of Compile() and Run()

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - Remove deprecated CreateDefaultPlatform()

[sergz]

fix layout/formating

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - *ToV8String() return MaybeLocal<> and check Call() return value

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - Use Maybe<> version of soon to be deprecated API in v8 6.7

[abpbot]

A commit referencing this issue has landed:
​Issue 6526 - Fix test by not exposing V8