Fix build breakage with Node.js 10.0.0-10.9.0.#833
Fix build breakage with Node.js 10.0.0-10.9.0.#833kkoopa merged 1 commit intonodejs:masterfrom bnoordhuis:fix832
Conversation
| #else | ||
| // See https://github.com/nodejs/nan/issues/832. | ||
| // Disable the warning as there is no way around it. | ||
| // TODO(bnoordhuis) Use isolate-based version in Node.js v12. |
There was a problem hiding this comment.
I think the isolate version of WriteUtf8 is already available since v11 so #if NODE_MAJOR_VERSION >= 11 should be fine here.
There was a problem hiding this comment.
What about
#if !NODE_VERSION_AT_LEAST(10,9,0)It will use the isolate-based version for 10.10 and higher.
There was a problem hiding this comment.
@MaartenBent There are situations where the node version used to build differs from that one used to execute the program. Therefore if you build with 10.10.0 but execute with 10.9.0 it will not work as the new API is not there. This is not intended as usually a native addon binary works for all minors of a major version.
There was a problem hiding this comment.
You're right, I didn't consider ABI compatibility.
There was a problem hiding this comment.
I think the isolate version of WriteUtf8 is already available since v11 so #if NODE_MAJOR_VERSION >= 11 should be fine here.
Right, that's true. Updated.
|
@bnoordhuis Do you think it would be a good idea to extend the test matrix with the initial node.js versions (e.g. add |
|
We could do that for 10, but doing it for others would lead to too large a test matrix.
Ideally it would not be necessary. I still do not see why changes of this nature are introduced in minor versions of Node, but that ship has sailed so we have to make the best of it.
…On December 17, 2018 11:05:16 PM GMT+02:00, "Gerhard Stöbich" ***@***.***> wrote:
@bnoordhuis Do you think it would be a good idea to extend the test
matrix with the initial node.js versions (e.g. add
`TRAVIS_NODE_VERSION="10.0.0"`,...)?
|
|
I would vote to add it for the "active" NodeJs versions, e.g. 10 and 11. I don't expect that anyone will touch older versions but I could imagine that the same deprecation mechanism used in Node 10 could happen again in 11 to prepare for 12. |
|
@kkoopa LGTY? Would be best to put out a bug fix release ASAP. |
|
Done, thank you both for triaging this. |
4 participants
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