reconbot
commented
5 years ago Good to know, thank you
Closed mcollina closed 5 years ago
reconbot
commented
5 years ago Good to know, thank you
reconbot
commented
5 years ago A collection of different types of upgrades we need to make.
We have a lot of calls to call that now needs an AsyncResource but I have no idea where that comes from.
https://github.com/node-serialport/node-serialport/blob/7c84e525979fc7252445c07928005cb2143a4ae2/packages/bindings/src/darwin_list.cpp#L352
Here we convert a string from js to a c string, and from an object to an object and there's a new way to do that I but I'm not sure what it is. https://github.com/node-serialport/node-serialport/blob/7c84e525979fc7252445c07928005cb2143a4ae2/packages/bindings/src/serialport.cpp#L37-L48
That might be everything!
zbjornson
commented
5 years ago Taking a shot at a PR now.
indutny
commented
5 years ago I don't think AsyncResource is absolutely necessary. You could just use v8::Local<v8::Value> Call(int argc, v8::Local<v8::Value> argv[]) const; as described in that nan API page.
Will take a look at object later.
indutny
commented
5 years ago Oh yikes. Didn't refresh the page since yesterday! :wink: :joy: Glad that it is resolved now!
fivdi
commented
5 years ago I don't think
AsyncResourceis absolutely necessary. You could just usev8::Local<v8::Value> Call(int argc, v8::Local<v8::Value> argv[]) const;as described in thatnanAPI page.
@indutny Are you sure AsyncResource isn't absolutely necessary? My assumption has been that AsyncResource is needed for asynchronous callbacks and that v8::Local<v8::Value> Call(int argc, v8::Local<v8::Value> argv[]) const; can only be used for synchronous callbacks. The callbacks that were modified in the PR corresponding to this issue all look like asynchronous callbacks so I think AsyncResource is needed.
zbjornson
commented
5 years ago This code I think actually should be updated to use AsyncResource. Without it the async context isn't propagated. I (sort of blindly) updated the syntax without adding AsyncResource. It's a simple change and an example of doing it correctly is here:
indutny
commented
5 years ago You're right, @fivdi and @zbjornson . Having AsyncResource in general should be a good idea. What I was implying is that its use wasn't necessary for making things compile again. Good feature request, though!
reconbot
commented
5 years ago Looking at N-API for when 6.0 gets end of lifed, but this might be a better approach https://github.com/nodejs/nan/issues/836
fivdi
commented
5 years ago @zbjornson I don't think the example linked to will function correctly as is creates the AsyncResource object directly before it makes the callback so it isn't creating the AsyncResource is the correct context. The AsyncResource resource should be created in the same context as the initial call to the function that accepted the callback as an argument.
@indutny Thanks for the feedback.
zbjornson
commented
5 years ago Hm possibly. @kkoopa made that change so I assumed it was correct (https://github.com/Automattic/node-canvas/commit/29087745722c17fb1894a1fc73bf30c0a48b0c11#diff-c76c7ea0c7b77ce0bd7e43541bf43e9dL498), but what you said makes sense.
fivdi
commented
5 years ago @kkoopa used to do this in the past but then @ofrobots pointed out that there is a better way to do it in this comment (this comment linked to here has been minimized so you'll need to expand it.)
zbjornson
commented
5 years ago Ah, thanks for pointing that out, will fix it in node-canvas also!
reconbot
commented
5 years ago @fivdi @zbjornson are we broken right now in prod?
fivdi
commented
5 years ago I haven't tried it but I would expect node-serialport#master to be good for everything up to but not including Node.js v12 nightly. For Node.js v12 nightly I would expect compile errors and warnings. If the nan dependency was changed from "^2.12.1" to "nodejs/nan" here in order to to pull in nan#master there should only be warnings with Node.js v12 nightly.
fivdi
commented
5 years ago Just to be clear, I'm not suggesting changing the nan dependency from "^2.12.1" to "nodejs/nan" in production. It would however need to be done to test with Node.js v12 nightly as it contains this nan commit that hasn't been released yet but is needed for Node.js v12 nightly.
reconbot
commented
5 years ago I can confirm that https://github.com/node-serialport/node-serialport/pull/1743 is causing issues, probably due to the stuff @fivdi highlighted. @zbjornson are you available to try to fix it?
zbjornson
commented
5 years ago @reconbot what issues specifically? I can look tonight in any case.
zbjornson
commented
5 years ago Ah, just saw the linked issue. Will take a look tonight, sorry!
reconbot
commented
5 years ago I'll take a look at https://github.com/node-serialport/node-serialport/pull/1765 but my c++ isn't very good so I can't provide a very good code review.
HipsterBrown
commented
5 years ago serialport@7.1.2 has been published to fix some recent issues. Can we confirm if it works for Node 12?
zbjornson
commented
5 years ago AFAIK, it will not work with Node 12 until a new version of Nan is released with the commit from https://github.com/nodejs/nan/pull/831. From the comments in that PR, it sounds like they're waiting until Node 12 is closer to being released.
jacobq
commented
5 years ago AFAIK, it will not work with Node 12 until a new version of Nan is released with the commit from nodejs/nan#831.
Looks like that merged in December 2018, and there have been multiple releases since then.
However, it looks like we need to update our deps to use ^2.13.2 here, right? https://github.com/node-serialport/node-serialport/blob/da63804ed3d65a28ef64ba6e7ba2c9ad23b23c97/packages/bindings/package.json#L13
Right now when I try to npm install serialport with node v12.0.0 (npm v6.9.0) in an empty directory I get an error during build:
jacobq
commented
5 years ago Actually, this appears to have nothing to do with the version of nan. The only error I see when trying to build against node v12 corresponds to a deprecation warning when trying to build again node v10.
../src/serialport.cpp:41:75: error: no matching function for call to ‘v8::String::Utf8Value::Utf8Value(v8::Local<v8::String>)’
v8::String::Utf8Value path(Nan::To<v8::String>(info[0]).ToLocalChecked());
^I don't know if this is the right solution (TBH, I don't even really know what it does), but it makes the error go away:
NAN_METHOD(Open) {
// **Added this**
v8::Isolate* isolate = info.GetIsolate();
// path
if (!info[0]->IsString()) {
Nan::ThrowTypeError("First argument must be a string");
return;
}
// **Changed this to include `isolate`**
v8::String::Utf8Value path(isolate, Nan::To<v8::String>(info[0]).ToLocalChecked());
// Used to be:
// v8::String::Utf8Value path(Nan::To<v8::String>(info[0]).ToLocalChecked());
//...Thanks to @jacobq (and @fivdi!) We now have https://www.npmjs.com/package/serialport/v/7.1.5 which includes the node 12 changes. I've also added node12 to our build pipeline.
From our tests, this module will not work on Node 12/master, likely for the usage of some deprecated V8 APIs.
We are currently removing this from our CITGM suite: https://github.com/nodejs/citgm/pull/654.