Add quotes for include_dir

[SurienDG]

• Added quotes for the include_dir path in index.js so it works correctly on both Windows and linux platforms (as mentioned in C1083: Cannot open include file: 'napi.h': No such file or directory node-addon-examples#183)

[RaisinTen]

Suggested change

	const include_dir = `"path.relative('.', __dirname)"`;
	const include_dir = `"${path.relative('.', __dirname)}"`;

Isn't this the intended change according to nodejs/node-addon-examples#183 (comment)?

[SurienDG]

You are correct. I meant to do that (must've been too tired 😅). Thanks for catching that.

Should be correct now

[mhdawson]

CI run to test across platforms: https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/4129/

[mhdawson]

This seems to fail across all platforms? - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/nodes=win-vs2017/4127/

[hanazuki]

This looks like the wrong way to fix the examples. include_dir was introduced in #766 to replace now-deprecated include and avoid gyp's list-context command expansion (<!@(command)), which fails to correctly escape paths containing spaces, quotes, etc.

Because the packages that already adopted include_dir expect its value to be a non-escaped path (whether they use gyp or not), changing its format breaks all the existing packages.

What we should do is to replace list-context expansion with scalar-context one. For example, 1_hello_world would be:

diff --git i/1_hello_world/node-addon-api/binding.gyp w/1_hello_world/node-addon-api/binding.gyp
index b819abb..ddf73d2 100644
--- i/1_hello_world/node-addon-api/binding.gyp
+++ w/1_hello_world/node-addon-api/binding.gyp
@@ -6,7 +6,7 @@
       "cflags_cc!": [ "-fno-exceptions" ],
       "sources": [ "hello.cc" ],
       "include_dirs": [
-        "<!@(node -p \"require('node-addon-api').include\")"
+        "<!(node -p \"require('node-addon-api').include_dir\")"
       ],
       'defines': [ 'NAPI_DISABLE_CPP_EXCEPTIONS' ],
     }
diff --git i/1_hello_world/node-addon-api/package.json w/1_hello_world/node-addon-api/package.json
index f70a69d..fb231dd 100644
--- i/1_hello_world/node-addon-api/package.json
+++ w/1_hello_world/node-addon-api/package.json
@@ -6,7 +6,7 @@
   "private": true,
   "dependencies": {
     "bindings": "~1.2.1",
-    "node-addon-api": "^1.0.0"
+    "node-addon-api": "^3.0.2"
   },
   "scripts": {
     "test": "node hello.js"

[mhdawson]

@hanazuki thanks for catching that. Moving the examples to use include_dir instead of include seems like the right way to go. @SurienDG FYI.

[SurienDG]

I see so if we get rid of the @ it should work on both platforms with the original include_dir? @hanazuki
As you said with

-        "<!@(node -p \"require('node-addon-api').include\")"
+        "<!(node -p \"require('node-addon-api').include_dir\")"

[hanazuki]

@SurienDG I think it should work on Windows as this repository itself is tested in that way on Windows CI environment. (sorry that I don't have a handy local Windows environment to check it).

Even if it doesn't, I suppose we need to fix the gyp's side since it has the responsibility to escape the given paths for each OS and toolchain it supports, and also because changing the node-addon-api side solely for gyp may break packages using the other build tools such as cmake-js.

[SurienDG]

Ah okay I'll give it a try and then update the examples (if you haven't already). Thanks for your advice @hanazuki

[mhdawson]

Based on the discussion I think this can be closed. Please let me know if you think that was not the right thing to do.