Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,11 @@
build/
node_modules/
Debug/
Release/
*.lock
*.log
npm-debug.log*

Comment thread
gengjiawen marked this conversation as resolved.
.idea/
.vscode/
.DS_Store
12 changes: 12 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
language: node_js
node_js:
- "8"
- "10"
# - "12" enable this when fix issue on Node.js 12
cache:
npm
before_script:
- npx envinfo
script:
- npm install
- npm test
8 changes: 6 additions & 2 deletions 6_object_wrap/nan/myobject.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ void MyObject::New(const Nan::FunctionCallbackInfo<v8::Value>& info) {
const int argc = 1;
v8::Local<v8::Value> argv[argc] = { info[0] };
v8::Local<v8::Function> cons = Nan::New<v8::Function>(constructor);
info.GetReturnValue().Set(cons->NewInstance(argc, argv));
v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
info.GetReturnValue().Set(
cons->NewInstance(context, argc, argv).ToLocalChecked());
}
}

Expand All @@ -61,5 +63,7 @@ void MyObject::Multiply(const Nan::FunctionCallbackInfo<v8::Value>& info) {
const int argc = 1;
v8::Local<v8::Value> argv[argc] = { Nan::New(obj->value_ * multiple) };

info.GetReturnValue().Set(cons->NewInstance(argc, argv));
v8::Local<v8::Context> context = info.GetIsolate()->GetCurrentContext();
info.GetReturnValue().Set(
cons->NewInstance(context, argc, argv).ToLocalChecked());
}
4 changes: 2 additions & 2 deletions 6_object_wrap/nan/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"private": true,
"gypfile": true,
"dependencies": {
"bindings": "~1.2.1",
"nan": "^2.0.0"
"bindings": "~1.5.0",
Comment thread
gengjiawen marked this conversation as resolved.
"nan": "^2.14.0"
}
}
5 changes: 4 additions & 1 deletion 7_factory_wrap/nan/myobject.cc
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ v8::Local<v8::Object> MyObject::NewInstance(v8::Local<v8::Value> arg) {
const unsigned argc = 1;
v8::Local<v8::Value> argv[argc] = { arg };
v8::Local<v8::Function> cons = Nan::New<v8::Function>(constructor);
v8::Local<v8::Object> instance = cons->NewInstance(argc, argv);
v8::Local<v8::Context> context =
v8::Isolate::GetCurrent()->GetCurrentContext();
v8::Local<v8::Object> instance =
cons->NewInstance(context, argc, argv).ToLocalChecked();

return scope.Escape(instance);
}
Expand Down
5 changes: 4 additions & 1 deletion 8_passing_wrapped/nan/myobject.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@ v8::Local<v8::Object> MyObject::NewInstance(v8::Local<v8::Value> arg) {
const unsigned argc = 1;
v8::Local<v8::Value> argv[argc] = { arg };
v8::Local<v8::Function> cons = Nan::New<v8::Function>(constructor);
v8::Local<v8::Object> instance = cons->NewInstance(argc, argv);
v8::Local<v8::Context> context =
v8::Isolate::GetCurrent()->GetCurrentContext();
v8::Local<v8::Object> instance =
cons->NewInstance(context, argc, argv).ToLocalChecked();

return scope.Escape(instance);
}
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Node.js Addon Examples
=========================================
[![Build Status](https://travis-ci.org/nodejs/node-addon-examples.svg?branch=master)](https://travis-ci.org/nodejs/node-addon-examples)

**A repository of [Node.js Addons](https://nodejs.org/api/addons.html#addons_c_addons) examples.**

Implementations of examples are named either after Node.js versions (`node_0.10`,
Expand Down
3 changes: 3 additions & 0 deletions async_work_thread_safe_function/node-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
"dependencies": {
"bindings": "~1.2.1"
},
"engines": {
"node": ">= 10.6.0"
},
Comment thread
gengjiawen marked this conversation as resolved.
"scripts": {
"test": "node index.js"
},
Expand Down
13 changes: 13 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "node-addon-examples",
"version": "1.0.0",
"description": "Node.js Addon Examples",
"main": "test_all.js",
"scripts": {
"test": "node test_all.js"
},
"dependencies": {
"chalk": "^2.4.2",
"semver": "^6.3.0"
}
}
49 changes: 49 additions & 0 deletions test_all.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
const fs = require('fs')
const path = require('path')
const { execSync } = require('child_process')
const chalk = require('chalk')
const semver = require('semver')

const excludeFolder = ['node_modules']

function getAllTests() {
return fs
.readdirSync('./')
.filter(i => {
return (
!i.startsWith('.') &&
fs.statSync(i).isDirectory() &&
!excludeFolder.includes(i)
)
})
.map(i => {
const p = path.join(__dirname, i)
const tests = fs
.readdirSync(p)
.filter(j => fs.statSync(path.join(p, j)).isDirectory())
.map(j => path.join(p, j))
return tests
})
}

getAllTests().map(tests => {
tests.map(i => {
console.log(chalk.green(`testing: ${i}`))
const p = require(path.join(i, 'package.json'))
if (p.engines && p.engines.node) {
const currentNodeVersion = process.versions.node
const range = p.engines.node
const engineOk = semver.satisfies(currentNodeVersion, range)
if (!engineOk) {
console.warn(
chalk.yellow(`${i} require Node.js ${range}, current is ${currentNodeVersion}, skipping`)
)
return
}
}
const stdout = execSync('npm install', {
cwd: i
})
console.log(stdout.toString())
})
})
3 changes: 3 additions & 0 deletions thread_safe_function_round_trip/node-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
"dependencies": {
"bindings": "~1.2.1"
},
"engines": {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as before, the real dependency is N-API version 4 which is supported by some version of 8.X as well. In any case this is still a good step.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mhdawson you're right and my recent experience push me to say that the most useful value to check if a feature is supported or not is the N-API version.

The problem is that it's not possible to check if a Node.js version is greater than XXX because for example the 8.16.0 has napi_thread_safe_function instead the 10.0.0 not.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering if we could specify multiple ranges maybe

">= 8.16.0 < 9.0.0 || >=10.6.0"

but not sure if that is allowed by the syntax.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's possible to specify multiple ranges. What is specified on the engines field has effect only if the engine-strict config flag is set.

npm --engine-strict install

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example failed on macOs Node.js 8.16.0, can you run it in other OS ?

gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
  CC(target) Release/obj.target/binding/binding.o
../binding.c:185:16: warning: type specifier missing, defaults to 'int' [-Wimplicit-int]
/*napi_value*/ NAPI_MODULE_INIT(/*napi_env env, napi_value exports*/) {
               ^
../binding.c:204:33: error: use of undeclared identifier 'env'
  assert(napi_define_properties(env, exports, 1, &start_work) == napi_ok);
                                ^
../binding.c:204:38: error: use of undeclared identifier 'exports'
  assert(napi_define_properties(env, exports, 1, &start_work) == napi_ok);
                                     ^
../binding.c:208:20: error: use of undeclared identifier 'env'
  assert(napi_wrap(env,
                   ^
../binding.c:209:20: error: use of undeclared identifier 'exports'
                   exports,
                   ^
../binding.c:216:10: error: use of undeclared identifier 'exports'
  return exports;
         ^
1 warning and 5 errors generated.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like no macro NAPI_MODULE_INIT on 8.x https://github.com/nodejs/node/blob/v8.x/src/node_api.h

Copy link
Copy Markdown
Member Author

@gengjiawen gengjiawen Aug 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor Module Initialization to NAPI_MODULE(NODE_GYP_MODULE_NAME, Init) works on 8.x.

Should we do it ? cc @gabrielschulhof

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAPI_MODULE_INIT being missing on v8.x may be an oversight as we expect same code to work across all the LTS versions subject to the NAPI_VERSION supported. Having said that,. not sure we'll be able to get that into the last v8.x release. Also need @gabrielschulhof to comment on whether than even makes sense. For now if we can refactor, maybe with an #ifdef so the example still shows the way we would want people to do it post 8.x ?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAPI_MODULE now works on all platform, #ifdef may not be necessary.

NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

"node": ">= 10.6.0"
},
"scripts": {
"test": "node index.js"
},
Expand Down