Incompatibility with Node 12/master #1742
Labels
Comments
9 tasks
|
Good to know, thank you |
|
A collection of different types of upgrades we need to make. We have a lot of calls to 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. That might be everything! |
|
Taking a shot at a PR now. |
Merged
|
I don't think Will take a look at object later. |
|
Oh yikes. Didn't refresh the page since yesterday! 😉 😂 Glad that it is resolved now! |
@indutny Are you sure |
|
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: |
|
You're right, @fivdi and @zbjornson . Having |
|
Looking at N-API for when 6.0 gets end of lifed, but this might be a better approach nodejs/nan#836 |
|
@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. |
|
Hm possibly. @kkoopa made that change so I assumed it was correct (Automattic/node-canvas@2908774#diff-c76c7ea0c7b77ce0bd7e43541bf43e9dL498), but what you said makes sense. |
|
Ah, thanks for pointing that out, will fix it in node-canvas also! |
|
@fivdi @zbjornson are we broken right now in prod? |
|
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 |
|
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. |
|
I can confirm that #1743 is causing issues, probably due to the stuff @fivdi highlighted. @zbjornson are you available to try to fix it? |
|
@reconbot what issues specifically? I can look tonight in any case. |
|
Ah, just saw the linked issue. Will take a look tonight, sorry! |
|
I'll take a look at #1765 but my c++ isn't very good so I can't provide a very good code review. |
|
|
|
AFAIK, it will not work with Node 12 until a new version of Nan is released with the commit from nodejs/nan#831. From the comments in that PR, it sounds like they're waiting until Node 12 is closer to being released. |
Looks like that merged in December 2018, and there have been multiple releases since then. Right now when I try to Exampledebug.log |
|
Actually, this appears to have nothing to do with the version of 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. |
2 tasks
reconbot
added a commit
to reconbot/citgm
that referenced
this issue
Apr 27, 2019
`serialport` now supports node 12. Closes: serialport/node-serialport#1742
targos
pushed a commit
to nodejs/citgm
that referenced
this issue
May 1, 2019
`serialport` now supports node 12. Closes: serialport/node-serialport#1742
tkurki
added a commit
to SignalK/signalk-server
that referenced
this issue
Sep 22, 2019
Serialport 7.1.5 is supposed to be compatible with Node 12, so let's upgrade to that to skip the ugly compile fails that people experience with Node 12. We are not supporting officially Node 12 though. serialport/node-serialport#1742 (comment)
tkurki
added a commit
to SignalK/signalk-server
that referenced
this issue
Sep 22, 2019
Serialport 7.1.5 is supposed to be compatible with Node 12, so let's upgrade to that to skip the ugly compile fails that people experience with Node 12. We are not supporting officially Node 12 though. serialport/node-serialport#1742 (comment)
tkurki
added a commit
to SignalK/signalk-server
that referenced
this issue
Sep 29, 2019
Serialport 7.1.5 is supposed to be compatible with Node 12, so let's upgrade to that to skip the ugly compile fails that people experience with Node 12. We are not supporting officially Node 12 though. serialport/node-serialport#1742 (comment)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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: nodejs/citgm#654.
The text was updated successfully, but these errors were encountered: