Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WriteUtf8 does not take 5 arguments #832

Closed
Sneezry opened this issue Dec 17, 2018 · 10 comments
Closed

WriteUtf8 does not take 5 arguments #832

Sneezry opened this issue Dec 17, 2018 · 10 comments

Comments

@Sneezry
Copy link

Sneezry commented Dec 17, 2018

Hi, I have no idea if I made some mistake. But I cannot rebuild nan with electron 3.0.10 (with node 10.2.0).

With command below to rebuild:

node-gyp rebuild --target=3.0.10 --arch=x64 --dist-url=https://atom.io/download/electron

The raw error I got is

C:\Users\...\node_modules\nan\nan.h(1081): error C2660: 'v8::String
::WriteUtf8': function does not take 5 arguments (compiling source file ..\src\combined.cpp) [C:\...\xxx.vcxproj]

I find that the latest nan will call WriteUtf8 with 5 arguments if node version is larger than 10

nan/nan.h

Line 1081 in 55923c8

length_ = string->WriteUtf8(v8::Isolate::GetCurrent(), str_, static_cast<int>(len), 0, flags);

However, WriteUtf8 only accepts 4 arguments in node 10

https://v8docs.nodesource.com/node-10.6/d2/db3/classv8_1_1_string.html#a99aec0d3b418feca1f2a2ba79416c60b

Any ideas? Thanks.

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

The 4 param version has been deprecated in v8 in favor of the 5 param version and this has been backported to node 10 (see nodejs/node#22531). Therefore the linked v8 docs don't match to the actual variant of v8 used by node.

See https://github.com/nodejs/node/blob/4128793a150dbcc63e03738ddd30f1768608aa11/deps/v8/include/v8.h#L2656

I assume that the same did not happen in electron and as a result some ifdefs are wrong in this setup.

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

Actually it seems that the backport of the v8 change was done with Node 10.10.0. Therefore if someone uses Nan 2.12.0 with Node 10.0.0 till 10.9.0 build will fail.

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

Caused by #825 - seems it's a decision between build errors and deprecation warnings.

@amb26
Copy link

amb26 commented Dec 17, 2018

This also fails in Node 10.11.0

@Flarna
Copy link
Member

Flarna commented Dec 17, 2018

@amb26 I guess you mean compiling electron with NodeJs 10.10.0. Or some native node extension?

@refack
Copy link
Contributor

refack commented Dec 17, 2018

@Flarna following your suggestion from TryGhost/node-sqlite3#1093 (review) I got this situation:

@bnoordhuis
Copy link
Member

I'll put up a PR with a fix shortly.

@bnoordhuis
Copy link
Member

#833 - would be great if an Electron user could test it out as well.

@refack
Copy link
Contributor

refack commented Dec 17, 2018

@GU5TAF
Copy link

GU5TAF commented Dec 18, 2018

For anyone like myself who's just trying to get their app to build whilst waiting for #833 to land.
If you're using yarn you can add nan@2.11.1 to the resolutions field in your package.json, like so:

"resolutions": {
  "nan": "2.11.1"
}

kkoopa pushed a commit that referenced this issue Dec 18, 2018
The five argument WriteUtf8() method was introduced in Node.js v10.0.0,
it doesn't exist in older 10.x releases.

Use the four argument overload and a bunch of pragmas to silence the
compiler warnings.

Fixes: #832
Refs: #825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants