Upgrade yarp.js to support node.js v14.17.0 #30
Merged
+1,381
−202
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Update cmake-js and socket.io to version "latest" in package.json => cmake-js resolves to v6.3.0. => socket.io resolves to v4.4.0 - Build is still failing
…JS_INIT macro definitions
…RPJS_BASE and YARPJS_INIT - Fixed error: 'no matching member function for call to NewInstance'. This was a mismatch in the arguments of v8::Local<v8::Function>.NewInstance()` due to an API change. - Fix error: 'no matching member function for call to 'Set' in macros YARPJS_BASE and YARPJS_INIT'. Add in `info.GetReturnValue().Set()` call the missing conversion from `Nan::MaybeLocal<>` to `v8::Local<>` using `Nan::MaybeLocal<v8::Object>::FromMaybe()`. - Remove duplicate definition of YARPJS_BASE and YARPJS_INIT macros.
|
Perhaps we may want to tag the exiting yarp.js as 1.0.1 or 1.1.0 ? |
|
Yes good point @traversaro. I don't know if there are any interface changes or significant features since v1.0.0. We have to check before choosing and applying the tag. |
We can also just automatically generate the changelog with the GitHub features. |
|
Hi @traversaro , I'm reading this: https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes. As a recap, the naming convention is: MAJOR.MINOR.PATCH
I also read here that it is a common practice to jump multiple minor versions in order to indicate that significant feature have been integrated. With this in mind, we could go for a tag like 1.5.0. What do you think @traversaro ? |
|
Good idea! |
- Since we don't have a particular default value to use in fromMaybe(...), we just convert the MaybeLocal<T> to Local<T>, when required, through the method ToLocalChecked() which returns an empty object if the wrapped variable is null. - If a v8::Value is expected instead of an v8::Object, we can still return an Object, (typically the case of Nan::NewInstance() method), since v8::Object derive from v8::Value (https://v8docs.nodesource.com/node-8.16/db/d85/classv8_1_1_object.html)
…v8::Object>' for 1st argument of 'Nan::ObjectWrap::Unwrap<...>(...)'
==== Fixed Errors:
- **error: too few arguments to function call, single argument 'context' was not specified**
Patterns causing the error:
```c++
Nan::FunctionCallbackInfo<v8::Value> &info
info[0]->ToString()
```
```c++
typedef const FunctionCallbackInfo<v8::Value>& NAN_METHOD_ARGS_TYPE;
#define NAN_METHOD(name) Nan::NAN_METHOD_RETURN_TYPE name(Nan::NAN_METHOD_ARGS_TYPE info)
NAN_METHOD(YarpJS_RPCPort::Open) {
...
v8::String::Utf8Value _port_name(info[0]->ToString());
...
}
```
Same use case with `info[0]->IntegerValue()`:
```c++
V8_WARN_UNUSED_RESULT Maybe<int64_t> IntegerValue(
Local<Context> context) const;
```
=> Add argument `Nan::GetCurrentContext()`.
- **error: no matching function for call to 'Unwrap'**
(no viable conversion from 'MaybeLocal<v8::Object>' to 'Local<v8::Object>')
Pattern causing the error:
```c++
YarpJS_Bottle* target = Nan::ObjectWrap::Unwrap<YarpJS_Bottle>(info[0]->ToObject(...));
```
=> Replace argument by `info[0]->ToObject(...).ToLocalChecked()`.
- **error: no matching constructor for initialization of 'v8::String::Utf8Value'**
(no known conversion from 'MaybeLocal<v8::String>' to 'const v8::String::Utf8Value' for 1st argument)
Prototype of the constructor changed from:
```c++
explicit Utf8Value(Local<v8::Value> obj);
```
to
```c++
Utf8Value(Isolate* isolate, Local<v8::Value> obj);
```
===== Note: class Isolate
Isolate represents an isolated instance of the V8 engine. V8 isolates have completely separate states. Objects from one isolate must not be used in other isolates. The embedder can create multiple isolates and use them in parallel in multiple threads. An isolate can be entered by at most one thread at any given time. The Locker/Unlocker API must be used to synchronize.
Use static method `Isolate::GetCurrent()`.
```c++
namespace v8
public: static Isolate *Isolate::GetCurrent()
Returns the entered isolate for the current thread or NULL in case there is no current isolate. This method must not be invoked before V8::Initialize() was invoked.
```
- **error: no viable conversion from 'Maybe<int64_t>' (aka 'Maybe<long long>') to 'int'**
Example:
```c++
int compression_quality = info[0]->IntegerValue(Nan::GetCurrentContext());
```
Since `info[0]->IntegerValue(Nan::GetCurrentContext())` returns `Maybe<int64_t>`, we need to convert the output to `int64_t`, using:
```c++
V8_INLINE T v8::Maybe::FromMaybe(const T& default_value) const {
return has_value ? value : default_value;
}
```
(refer to class `Maybe` definition in `v8.h`).
==== Fixed Warnings:
- **warning: 'Mutex' is deprecated: Use std::mutex instead**
- **warning: 'tryLock' is deprecated: Use try_lock() instead**
==== Non fixed warnings:
- **warning: 'Call' is deprecated [-Wdeprecated-declarations]**
Pattern causing the error:
```c++
tmp_this->callback->Call(tmp_arguments.size(),tmp_arguments.data());
```
--> in `yarp.js/YarpJS/include/YarpJS_Callback.h:124:25`.
- **warning: address of function 'Nan::Null' will always evaluate to 'true'**
Example:
```c++
v8::Local<v8::Value> argv[1] = {Nan::New(Nan::Null)};
```
--> in `yarp.js/YarpJS/src/YarpJS_BufferedPort_Bottle.cpp:93:44`.
--> in `yarp.js/YarpJS/src/YarpJS_BufferedPort_Sound.cpp:23:46`.
- **warning: 'getIplImage' is deprecated: Use yarp::cv::toCvMat instead**
Example:
```c++
internalImage = cv::cvarrToMat((IplImage *) this->getYarpObj()->getIplImage());
```
--> in `yarp.js/YarpJS/src/YarpJS_Image.cpp:23:69`.
Required Node.js version changed.
|
CC @Nicogene |
traversaro
reviewed
Dec 20, 2021
traversaro
approved these changes
Dec 20, 2021
|
@traversaro , I've just added the tag 1.5.0. The release notes were generated automatically, to which I added a line for a set set of old changes merged without PRs: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements #28 .