From c79cabaed25fd341b46ddabf44a7cd1056bdc397 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 3 Nov 2020 00:08:59 +0100 Subject: [PATCH 1/9] doc: avoid directing users to HTTP (#828) --- README.md | 4 ++-- doc/error.md | 2 +- doc/hierarchy.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index f88ac6871..8e7ee391d 100644 --- a/README.md +++ b/README.md @@ -37,8 +37,8 @@ APIs exposed by node-addon-api are generally used to create and manipulate JavaScript values. Concepts and operations generally map to ideas specified in the **ECMA262 Language Specification**. -The [N-API Resource](http://nodejs.github.io/node-addon-examples/) offers an -excellent orientation and tips for developers just getting started with N-API +The [N-API Resource](https://nodejs.github.io/node-addon-examples/) offers an +excellent orientation and tips for developers just getting started with N-API and node-addon-api. - **[Setup](#setup)** diff --git a/doc/error.md b/doc/error.md index 7530262d7..08c156dd0 100644 --- a/doc/error.md +++ b/doc/error.md @@ -117,4 +117,4 @@ Returns a pointer to a null-terminated string that is used to identify the exception. This method can be used only if the exception mechanism is enabled. [`Napi::ObjectReference`]: ./object_reference.md -[`std::exception`]: http://cplusplus.com/reference/exception/exception/ +[`std::exception`]: https://cplusplus.com/reference/exception/exception/ diff --git a/doc/hierarchy.md b/doc/hierarchy.md index e40a65ff9..921f94a99 100644 --- a/doc/hierarchy.md +++ b/doc/hierarchy.md @@ -88,4 +88,4 @@ [`Napi::Uint8Array`]: ./typed_array_of.md [`Napi::Value`]: ./value.md [`Napi::VersionManagement`]: ./version_management.md -[`std::exception`]: http://cplusplus.com/reference/exception/exception/ +[`std::exception`]: https://cplusplus.com/reference/exception/exception/ From c9563caa25bfb208f4dd8f0f0e55f0861bf057dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Sun, 26 Jan 2020 00:30:38 -0400 Subject: [PATCH 2/9] src: add ArrayBuffer::Detach() and ::IsDetached() Refs: https://github.com/nodejs/node/pull/29768 Refs: https://github.com/nodejs/node/pull/30613 PR-URL: https://github.com/nodejs/node-addon-api/pull/659 Refs: https://github.com/nodejs/node/pull/29768 Refs: https://github.com/nodejs/node/pull/30613 Reviewed-By: Michael Dawson Reviewed-By: Gabriel Schulhof --- doc/array_buffer.md | 16 ++++++++++++++++ napi-inl.h | 14 ++++++++++++++ napi.h | 5 +++++ test/arraybuffer.cc | 38 +++++++++++++++++++++++++++++++------- test/arraybuffer.js | 5 +++++ 5 files changed, 71 insertions(+), 7 deletions(-) diff --git a/doc/array_buffer.md b/doc/array_buffer.md index 988a839a4..346fe6ace 100644 --- a/doc/array_buffer.md +++ b/doc/array_buffer.md @@ -130,4 +130,20 @@ void* Napi::ArrayBuffer::Data() const; Returns a pointer the wrapped data. +### Detach + +```cpp +void Napi::ArrayBuffer::Detach(); +``` + +Invokes the `ArrayBuffer` detach operation on a detachable `ArrayBuffer`. + +### IsDetached + +```cpp +bool Napi::ArrayBuffer::IsDetached() const; +``` + +Returns `true` if this `ArrayBuffer` has been detached. + [`Napi::Object`]: ./object.md diff --git a/napi-inl.h b/napi-inl.h index 5ecdd5046..5866fd89c 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -1530,6 +1530,20 @@ inline size_t ArrayBuffer::ByteLength() { return length; } +#if NAPI_VERSION >= 7 +inline bool ArrayBuffer::IsDetached() const { + bool detached; + napi_status status = napi_is_detached_arraybuffer(_env, _value, &detached); + NAPI_THROW_IF_FAILED(_env, status, false); + return detached; +} + +inline void ArrayBuffer::Detach() { + napi_status status = napi_detach_arraybuffer(_env, _value); + NAPI_THROW_IF_FAILED_VOID(_env, status); +} +#endif // NAPI_VERSION >= 7 + //////////////////////////////////////////////////////////////////////////////// // DataView class //////////////////////////////////////////////////////////////////////////////// diff --git a/napi.h b/napi.h index 76a62f659..945aac55e 100644 --- a/napi.h +++ b/napi.h @@ -822,6 +822,11 @@ namespace Napi { void* Data(); ///< Gets a pointer to the data buffer. size_t ByteLength(); ///< Gets the length of the array buffer in bytes. + +#if NAPI_VERSION >= 7 + bool IsDetached() const; + void Detach(); +#endif // NAPI_VERSION >= 7 }; /// A JavaScript typed-array value with unknown array type. diff --git a/test/arraybuffer.cc b/test/arraybuffer.cc index cc4f1f51c..4a1b2a34e 100644 --- a/test/arraybuffer.cc +++ b/test/arraybuffer.cc @@ -157,19 +157,43 @@ void CheckDetachUpdatesData(const CallbackInfo& info) { return; } - if (!info[1].IsFunction()) { - Error::New(info.Env(), "A function was expected.").ThrowAsJavaScriptException(); - return; - } - ArrayBuffer buffer = info[0].As(); - Function detach = info[1].As(); // This potentially causes the buffer to cache its data pointer and length. buffer.Data(); buffer.ByteLength(); - detach.Call({}); +#if NAPI_VERSION >= 7 + if (buffer.IsDetached()) { + Error::New(info.Env(), "Buffer should not be detached.").ThrowAsJavaScriptException(); + return; + } +#endif + + if (info.Length() == 2) { + // Detach externally (in JavaScript). + if (!info[1].IsFunction()) { + Error::New(info.Env(), "A function was expected.").ThrowAsJavaScriptException(); + return; + } + + Function detach = info[1].As(); + detach.Call({}); + } else { +#if NAPI_VERSION >= 7 + // Detach directly. + buffer.Detach(); +#else + return; +#endif + } + +#if NAPI_VERSION >= 7 + if (!buffer.IsDetached()) { + Error::New(info.Env(), "Buffer should be detached.").ThrowAsJavaScriptException(); + return; + } +#endif if (buffer.Data() != nullptr) { Error::New(info.Env(), "Incorrect data pointer.").ThrowAsJavaScriptException(); diff --git a/test/arraybuffer.js b/test/arraybuffer.js index 38d35d1e7..363de17d9 100644 --- a/test/arraybuffer.js +++ b/test/arraybuffer.js @@ -58,8 +58,13 @@ function test(binding) { 'ArrayBuffer updates data pointer and length when detached', () => { + // Detach the ArrayBuffer in JavaScript. const mem = new WebAssembly.Memory({ initial: 1 }); binding.arraybuffer.checkDetachUpdatesData(mem.buffer, () => mem.grow(1)); + + // Let C++ detach the ArrayBuffer. + const extBuffer = binding.arraybuffer.createExternalBuffer(); + binding.arraybuffer.checkDetachUpdatesData(extBuffer); }, ]); } From 1b52c28eb810ea46c2d5579cb16e8b6f1beb3869 Mon Sep 17 00:00:00 2001 From: mastergberry Date: Mon, 9 Nov 2020 17:57:35 +0100 Subject: [PATCH 3/9] Clean up AsyncProgressWorker documentation (#831) * Clean up AsyncProgressWorker documentation The old documentation was not very clear on how to use the API. It was missing javascript references, and was not necessarily following the best coding standards. These adjustments make it a bit more complete now by providing all three necessary parts, the worker, the hookup code, and the javascript usage of it. I have also gone ahead and rewritten the `AsyncProgressQueueWorker` example to show demonstration of the `FunctionReference` class and how to use multiple callbacks instead of one combined callback. --- doc/async_worker_variants.md | 181 +++++++++++++++++++++++++++-------- 1 file changed, 141 insertions(+), 40 deletions(-) diff --git a/doc/async_worker_variants.md b/doc/async_worker_variants.md index 8b892eaee..54007762c 100644 --- a/doc/async_worker_variants.md +++ b/doc/async_worker_variants.md @@ -243,7 +243,9 @@ be safely released. Note that `Napi::AsyncProgressWorker::ExecutionProcess::Send` merely guarantees **eventual** invocation of `Napi::AsyncProgressWorker::OnProgress`, which means multiple send might be coalesced into single invocation of `Napi::AsyncProgressWorker::OnProgress` -with latest data. +with latest data. If you would like to guarantee that there is one invocation of +`OnProgress` for every `Send` call, you should use the `Napi::AsyncProgressQueueWorker` +class instead which is documented further down this page. ```cpp void Napi::AsyncProgressWorker::ExecutionProcess::Send(const T* data, size_t count) const; @@ -269,7 +271,8 @@ function runs in the background out of the **event loop** thread and at the end the `Napi::AsyncProgressWorker::OnOK` or `Napi::AsyncProgressWorker::OnError` function will be called and are executed as part of the event loop. -The code below shows a basic example of the `Napi::AsyncProgressWorker` implementation: +The code below shows a basic example of the `Napi::AsyncProgressWorker` implementation along with an +example of how the counterpart in Javascript would appear: ```cpp #include @@ -281,28 +284,38 @@ using namespace Napi; class EchoWorker : public AsyncProgressWorker { public: - EchoWorker(Function& callback, std::string& echo) - : AsyncProgressWorker(callback), echo(echo) {} + EchoWorker(Function& okCallback, std::string& echo) + : AsyncProgressWorker(okCallback), echo(echo) {} ~EchoWorker() {} - // This code will be executed on the worker thread - void Execute(const ExecutionProgress& progress) { - // Need to simulate cpu heavy task - for (uint32_t i = 0; i < 100; ++i) { - progress.Send(&i, 1) - std::this_thread::sleep_for(std::chrono::seconds(1)); + + // This code will be executed on the worker thread + void Execute(const ExecutionProgress& progress) { + // Need to simulate cpu heavy task + // Note: This Send() call is not guaranteed to trigger an equal + // number of OnProgress calls (read documentation above for more info) + for (uint32_t i = 0; i < 100; ++i) { + progress.Send(&i, 1) + } + } + + void OnError(const Error &e) { + HandleScope scope(Env()); + // Pass error onto JS, no data for other parameters + Callback().Call({String::New(Env(), e.Message())}); } - } - void OnOK() { - HandleScope scope(Env()); - Callback().Call({Env().Null(), String::New(Env(), echo)}); - } + void OnOK() { + HandleScope scope(Env()); + // Pass no error, give back original data + Callback().Call({Env().Null(), String::New(Env(), echo)}); + } - void OnProgress(const uint32_t* data, size_t /* count */) { - HandleScope scope(Env()); - Callback().Call({Env().Null(), Env().Null(), Number::New(Env(), *data)}); - } + void OnProgress(const uint32_t* data, size_t /* count */) { + HandleScope scope(Env()); + // Pass no error, no echo data, but do pass on the progress data + Callback().Call({Env().Null(), Env().Null(), Number::New(Env(), *data)}); + } private: std::string echo; @@ -327,12 +340,23 @@ using namespace Napi; Value Echo(const CallbackInfo& info) { // We need to validate the arguments here - Function cb = info[1].As(); std::string in = info[0].As(); + Function cb = info[1].As(); EchoWorker* wk = new EchoWorker(cb, in); wk->Queue(); return info.Env().Undefined(); } + +// Register the native method for JS to access +Object Init(Env env, Object exports) +{ + exports.Set(String::New(env, "echo"), Function::New(env, Echo)); + + return exports; +} + +// Register our native addon +NODE_API_MODULE(nativeAddon, Init) ``` The implementation of a `Napi::AsyncProgressWorker` can be used by creating a @@ -341,6 +365,20 @@ asynchronous task ends and other data needed for the computation. Once created, the only other action needed is to call the `Napi::AsyncProgressWorker::Queue` method that will queue the created worker for execution. +Lastly, the following Javascript (ES6+) code would be associated the above example: + +```js +const { nativeAddon } = require('binding.node'); + +const exampleCallback = (errorResponse, okResponse, progressData) => { + // Use the data accordingly + // ... +}; + +// Call our native addon with the paramters of a string and a function +nativeAddon.echo("example", exampleCallback); +``` + # AsyncProgressQueueWorker `Napi::AsyncProgressQueueWorker` acts exactly like `Napi::AsyncProgressWorker` @@ -379,7 +417,9 @@ void Napi::AsyncProgressQueueWorker::ExecutionProcess::Send(const T* data, size_ ## Example -The code below shows a basic example of the `Napi::AsyncProgressQueueWorker` implementation: +The code below show an example of the `Napi::AsyncProgressQueueWorker` implementation, but +also demonsrates how to use multiple `Napi::Function`'s if you wish to provide multiple +callback functions for more object oriented code: ```cpp #include @@ -391,31 +431,55 @@ using namespace Napi; class EchoWorker : public AsyncProgressQueueWorker { public: - EchoWorker(Function& callback, std::string& echo) - : AsyncProgressQueueWorker(callback), echo(echo) {} + EchoWorker(Function& okCallback, Function& errorCallback, Function& progressCallback, std::string& echo) + : AsyncProgressQueueWorker(okCallback), echo(echo) { + // Set our function references to use them below + this->errorCallback.Reset(errorCallback, 1); + this->progressCallback.Reset(progressCallback, 1); + } ~EchoWorker() {} - // This code will be executed on the worker thread - void Execute(const ExecutionProgress& progress) { - // Need to simulate cpu heavy task - for (uint32_t i = 0; i < 100; ++i) { - progress.Send(&i, 1); - std::this_thread::sleep_for(std::chrono::seconds(1)); + + // This code will be executed on the worker thread + void Execute(const ExecutionProgress& progress) { + // Need to simulate cpu heavy task to demonstrate that + // every call to Send() will trigger an OnProgress function call + for (uint32_t i = 0; i < 100; ++i) { + progress.Send(&i, 1); + } } - } - void OnOK() { - HandleScope scope(Env()); - Callback().Call({Env().Null(), String::New(Env(), echo)}); - } + void OnOK() { + HandleScope scope(Env()); + // Call our onOkCallback in javascript with the data we were given originally + Callback().Call({String::New(Env(), echo)}); + } + + void OnError(const Error &e) { + HandleScope scope(Env()); + + // We call our callback provided in the constructor with 2 parameters + if (!this->errorCallback.IsEmpty()) { + // Call our onErrorCallback in javascript with the error message + this->errorCallback.Call(Receiver().Value(), {String::New(Env(), e.Message())}); + } + } - void OnProgress(const uint32_t* data, size_t /* count */) { - HandleScope scope(Env()); - Callback().Call({Env().Null(), Env().Null(), Number::New(Env(), *data)}); - } + void OnProgress(const uint32_t* data, size_t /* count */) { + HandleScope scope(Env()); + + if (!this->progressCallback.IsEmpty()) { + // Call our onProgressCallback in javascript with each integer from 0 to 99 (inclusive) + // as this function is triggered from the above Send() calls + this->progressCallback.Call(Receiver().Value(), {Number::New(Env(), *data)}); + } + } private: std::string echo; + FunctionReference progressCallback; + FunctionReference errorCallback; + }; ``` @@ -439,12 +503,25 @@ using namespace Napi; Value Echo(const CallbackInfo& info) { // We need to validate the arguments here. - Function cb = info[1].As(); std::string in = info[0].As(); - EchoWorker* wk = new EchoWorker(cb, in); + Function errorCb = info[1].As(); + Function okCb = info[2].As(); + Function progressCb = info[3].As(); + EchoWorker* wk = new EchoWorker(okCb, errorCb, progressCb, in); wk->Queue(); return info.Env().Undefined(); } + +// Register the native method for JS to access +Object Init(Env env, Object exports) +{ + exports.Set(String::New(env, "echo"), Function::New(env, Echo)); + + return exports; +} + +// Register our native addon +NODE_API_MODULE(nativeAddon, Init) ``` The implementation of a `Napi::AsyncProgressQueueWorker` can be used by creating a @@ -453,4 +530,28 @@ asynchronous task ends and other data needed for the computation. Once created, the only other action needed is to call the `Napi::AsyncProgressQueueWorker::Queue` method that will queue the created worker for execution. +Lastly, the following Javascript (ES6+) code would be associated the above example: + +```js +const { nativeAddon } = require('binding.node'); + +const onErrorCallback = (msg) => { + // Use the data accordingly + // ... +}; + +const onOkCallback = (echo) => { + // Use the data accordingly + // ... +}; + +const onProgressCallback = (num) => { + // Use the data accordingly + // ... +}; + +// Call our native addon with the paramters of a string and three callback functions +nativeAddon.echo("example", onErrorCallback, onOkCallback, onProgressCallback); +``` + [`Napi::AsyncWorker`]: ./async_worker.md From 2c02d317e51058692042406a550ffb24cee937db Mon Sep 17 00:00:00 2001 From: Kevin Eady <8634912+KevinEady@users.noreply.github.com> Date: Mon, 9 Nov 2020 17:58:32 +0100 Subject: [PATCH 4/9] build: add pre-commit package for linting (#823) * build: add incremental clang-format checks * build: add pre-commit package for linting --- .clang-format | 111 ++++++++++++++++++++++++++++++++++++++++ .travis.yml | 1 + package.json | 9 +++- scripts/clang-format.js | 42 +++++++++++++++ 4 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 .clang-format create mode 100644 scripts/clang-format.js diff --git a/.clang-format b/.clang-format new file mode 100644 index 000000000..4aad29c32 --- /dev/null +++ b/.clang-format @@ -0,0 +1,111 @@ +--- +Language: Cpp +# BasedOnStyle: Google +AccessModifierOffset: -1 +AlignAfterOpenBracket: Align +AlignConsecutiveAssignments: false +AlignConsecutiveDeclarations: false +AlignEscapedNewlines: Right +AlignOperands: true +AlignTrailingComments: true +AllowAllParametersOfDeclarationOnNextLine: true +AllowShortBlocksOnASingleLine: false +AllowShortCaseLabelsOnASingleLine: false +AllowShortFunctionsOnASingleLine: Inline +AllowShortIfStatementsOnASingleLine: true +AllowShortLoopsOnASingleLine: true +AlwaysBreakAfterDefinitionReturnType: None +AlwaysBreakAfterReturnType: None +AlwaysBreakBeforeMultilineStrings: false +AlwaysBreakTemplateDeclarations: true +BinPackArguments: false +BinPackParameters: false +BraceWrapping: + AfterClass: false + AfterControlStatement: false + AfterEnum: false + AfterFunction: false + AfterNamespace: false + AfterObjCDeclaration: false + AfterStruct: false + AfterUnion: false + AfterExternBlock: false + BeforeCatch: false + BeforeElse: false + IndentBraces: false + SplitEmptyFunction: true + SplitEmptyRecord: true + SplitEmptyNamespace: true +BreakBeforeBinaryOperators: None +BreakBeforeBraces: Attach +BreakBeforeInheritanceComma: false +BreakBeforeTernaryOperators: true +BreakConstructorInitializersBeforeComma: false +BreakConstructorInitializers: BeforeColon +BreakAfterJavaFieldAnnotations: false +BreakStringLiterals: true +ColumnLimit: 80 +CommentPragmas: '^ IWYU pragma:' +CompactNamespaces: false +ConstructorInitializerAllOnOneLineOrOnePerLine: true +ConstructorInitializerIndentWidth: 4 +ContinuationIndentWidth: 4 +Cpp11BracedListStyle: true +DerivePointerAlignment: false +DisableFormat: false +ExperimentalAutoDetectBinPacking: false +FixNamespaceComments: true +ForEachMacros: + - foreach + - Q_FOREACH + - BOOST_FOREACH +IncludeBlocks: Preserve +IncludeCategories: + - Regex: '^' + Priority: 2 + - Regex: '^<.*\.h>' + Priority: 1 + - Regex: '^<.*' + Priority: 2 + - Regex: '.*' + Priority: 3 +IncludeIsMainRegex: '([-_](test|unittest))?$' +IndentCaseLabels: true +IndentPPDirectives: None +IndentWidth: 2 +IndentWrappedFunctionNames: false +JavaScriptQuotes: Leave +JavaScriptWrapImports: true +KeepEmptyLinesAtTheStartOfBlocks: false +MacroBlockBegin: '' +MacroBlockEnd: '' +MaxEmptyLinesToKeep: 1 +NamespaceIndentation: None +ObjCBlockIndentWidth: 2 +ObjCSpaceAfterProperty: false +ObjCSpaceBeforeProtocolList: false +PenaltyBreakAssignment: 2 +PenaltyBreakBeforeFirstCallParameter: 1 +PenaltyBreakComment: 300 +PenaltyBreakFirstLessLess: 120 +PenaltyBreakString: 1000 +PenaltyExcessCharacter: 1000000 +PenaltyReturnTypeOnItsOwnLine: 200 +PointerAlignment: Left +ReflowComments: true +SortIncludes: true +SortUsingDeclarations: true +SpaceAfterCStyleCast: false +SpaceAfterTemplateKeyword: true +SpaceBeforeAssignmentOperators: true +SpaceBeforeParens: ControlStatements +SpaceInEmptyParentheses: false +SpacesBeforeTrailingComments: 2 +SpacesInAngles: false +SpacesInContainerLiterals: true +SpacesInCStyleCastParentheses: false +SpacesInParentheses: false +SpacesInSquareBrackets: false +Standard: Auto +TabWidth: 8 +UseTab: Never diff --git a/.travis.yml b/.travis.yml index 56f9c903c..642470330 100644 --- a/.travis.yml +++ b/.travis.yml @@ -54,6 +54,7 @@ script: # Travis CI sets NVM_NODEJS_ORG_MIRROR, but it makes node-gyp fail to download headers for nightly builds. - unset NVM_NODEJS_ORG_MIRROR + - npm run lint - npm test after_success: - cpp-coveralls --gcov-options '\-lp' --build-root test/build --exclude test diff --git a/package.json b/package.json index 93f3fd81c..2d25d0aaa 100644 --- a/package.json +++ b/package.json @@ -264,8 +264,10 @@ "description": "Node.js API (N-API)", "devDependencies": { "benchmark": "^2.1.4", - "fs-extra": "^9.0.1", "bindings": "^1.5.0", + "clang-format": "^1.4.0", + "fs-extra": "^9.0.1", + "pre-commit": "^1.2.2", "safe-buffer": "^5.1.1" }, "directories": {}, @@ -300,7 +302,10 @@ "dev": "node test", "predev:incremental": "node-gyp configure build -C test --debug", "dev:incremental": "node test", - "doc": "doxygen doc/Doxyfile" + "doc": "doxygen doc/Doxyfile", + "lint": "node scripts/clang-format.js", + "lint:fix": "git-clang-format" }, + "pre-commit": "lint", "version": "3.0.2" } diff --git a/scripts/clang-format.js b/scripts/clang-format.js new file mode 100644 index 000000000..b44d47674 --- /dev/null +++ b/scripts/clang-format.js @@ -0,0 +1,42 @@ +#!/usr/bin/env node + +const spawn = require('child_process').spawnSync; +const path = require('path'); + +const filesToCheck = ['*.h', '*.cc']; +const CLANG_FORMAT_START = process.env.CLANG_FORMAT_START || 'master'; + +function main(args) { + let clangFormatPath = path.dirname(require.resolve('clang-format')); + const options = ['--binary=node_modules/.bin/clang-format', '--style=file']; + + const gitClangFormatPath = path.join(clangFormatPath, + 'bin/git-clang-format'); + const result = spawn('python', [ + gitClangFormatPath, + ...options, + '--diff', + CLANG_FORMAT_START, + 'HEAD', + ...filesToCheck + ], { encoding: 'utf-8' }); + + if (result.error) { + console.error('Error running git-clang-format:', result.error); + return 2; + } + + const clangFormatOutput = result.stdout.trim(); + if (clangFormatOutput !== ('no modified files to format') && + clangFormatOutput !== ('clang-format did not modify any files')) { + console.error(clangFormatOutput); + const fixCmd = '"npm run lint:fix"'; + console.error(` + ERROR: please run ${fixCmd} to format changes in your commit`); + return 1; + } +} + +if (require.main === module) { + process.exitCode = main(process.argv.slice(2)); +} From 8fb5820557af11809dc1278518d2c96f001451bd Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 8 Oct 2020 01:47:20 +0800 Subject: [PATCH 5/9] build: add incremental clang-format checks PR-URL: https://github.com/nodejs/node-addon-api/pull/819 Reviewed-By: Gabriel Schulhof Reviewed-By: Michael Dawson --- .github/workflows/linter.yml | 23 +++++++++++++++++++++++ .travis.yml | 1 - 2 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/linter.yml diff --git a/.github/workflows/linter.yml b/.github/workflows/linter.yml new file mode 100644 index 000000000..3cbf00634 --- /dev/null +++ b/.github/workflows/linter.yml @@ -0,0 +1,23 @@ +name: Style Checks + +on: [push, pull_request] + +jobs: + lint: + strategy: + matrix: + node-version: [14.x] + os: [ubuntu-latest] + + runs-on: ${{ matrix.os }} + steps: + - uses: actions/checkout@v2 + with: + fetch-depth: 0 + - run: git branch -a + - name: Use Node.js ${{ matrix.node-version }} + uses: actions/setup-node@v1 + with: + node-version: ${{ matrix.node-version }} + - run: npm install + - run: CLANG_FORMAT_START=refs/remotes/origin/master npm run lint diff --git a/.travis.yml b/.travis.yml index 642470330..56f9c903c 100644 --- a/.travis.yml +++ b/.travis.yml @@ -54,7 +54,6 @@ script: # Travis CI sets NVM_NODEJS_ORG_MIRROR, but it makes node-gyp fail to download headers for nightly builds. - unset NVM_NODEJS_ORG_MIRROR - - npm run lint - npm test after_success: - cpp-coveralls --gcov-options '\-lp' --build-root test/build --exclude test From 1427b3ef7853271bfdf9057c33ab9a03e7fc67b1 Mon Sep 17 00:00:00 2001 From: blagoev Date: Mon, 16 Nov 2020 23:09:02 +0200 Subject: [PATCH 6/9] src: create a HandleScope in FinalizeCallback Refs: https://github.com/nodejs/node-addon-api/pull/832 Seems like FinalizeCallback needs to create a HandleScope since it calls ObjectWrap::~ObjectWrap() which might need to create a temporary Object handle here https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3558 at Realm (https://github.com/realm/realm-js) we have crashes with stacktrace at this location. While fixed in core for later versions we still need for older versions of Node.js PR-URL: https://github.com/nodejs/node-addon-api/pull/832 Reviewed-By: Gabriel Schulhof Reviewed-By: Kevin Eady Reviewed-By: Michael Dawson Reviewed-By: NickNaso --- napi-inl.h | 1 + 1 file changed, 1 insertion(+) diff --git a/napi-inl.h b/napi-inl.h index 5866fd89c..ab7ece1c8 100644 --- a/napi-inl.h +++ b/napi-inl.h @@ -3977,6 +3977,7 @@ inline napi_value ObjectWrap::StaticSetterCallbackWrapper( template inline void ObjectWrap::FinalizeCallback(napi_env env, void* data, void* /*hint*/) { + HandleScope scope(env); T* instance = static_cast(data); instance->Finalize(Napi::Env(env)); delete instance; From 59c6a6aeb0bfd8e488fa0975af23f1219e41adde Mon Sep 17 00:00:00 2001 From: legendecas Date: Sat, 21 Nov 2020 03:43:56 +0800 Subject: [PATCH 7/9] fix: git-clang-format doesn't recognize no changes requested on given files (#835) --- package.json | 2 +- tools/README.md | 8 +++++++- {scripts => tools}/clang-format.js | 3 ++- 3 files changed, 10 insertions(+), 3 deletions(-) rename {scripts => tools}/clang-format.js (92%) diff --git a/package.json b/package.json index 2d25d0aaa..85d6ea2d1 100644 --- a/package.json +++ b/package.json @@ -303,7 +303,7 @@ "predev:incremental": "node-gyp configure build -C test --debug", "dev:incremental": "node test", "doc": "doxygen doc/Doxyfile", - "lint": "node scripts/clang-format.js", + "lint": "node tools/clang-format.js", "lint:fix": "git-clang-format" }, "pre-commit": "lint", diff --git a/tools/README.md b/tools/README.md index 8d110609c..a1024d586 100644 --- a/tools/README.md +++ b/tools/README.md @@ -1,4 +1,10 @@ -# Migration Script +# Tools + +## clang-format + +The clang-format checking tools is designed to check changed lines of code compared to given git-refs. + +## Migration Script The migration tool is designed to reduce repetitive work in the migration process. However, the script is not aiming to convert every thing for you. There are usually some small fixes and major reconstruction required. diff --git a/scripts/clang-format.js b/tools/clang-format.js similarity index 92% rename from scripts/clang-format.js rename to tools/clang-format.js index b44d47674..3a2ba4a2f 100644 --- a/scripts/clang-format.js +++ b/tools/clang-format.js @@ -27,7 +27,8 @@ function main(args) { } const clangFormatOutput = result.stdout.trim(); - if (clangFormatOutput !== ('no modified files to format') && + if (clangFormatOutput !== '' && + clangFormatOutput !== ('no modified files to format') && clangFormatOutput !== ('clang-format did not modify any files')) { console.error(clangFormatOutput); const fixCmd = '"npm run lint:fix"'; From 6321f2ba1a66abca8abfb80011a93dc91bb8a729 Mon Sep 17 00:00:00 2001 From: raisinten Date: Thu, 19 Nov 2020 18:46:40 +0530 Subject: [PATCH 8/9] test: fix typos in addon_build/index.js PR-URL: https://github.com/nodejs/node-addon-api/pull/838 Reviewed-By: Michael Dawson Reviewed-By: Chengzhong Wu Reviewed-By: NickNaso --- test/addon_build/index.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/addon_build/index.js b/test/addon_build/index.js index 0d0c6ea2f..c410bf3a8 100644 --- a/test/addon_build/index.js +++ b/test/addon_build/index.js @@ -1,7 +1,7 @@ 'use strict'; const { promisify } = require('util'); -const exec = promisify(require('child_process').exec); +const exec = promisify(require('child_process').exec); const { copy, remove } = require('fs-extra'); const path = require('path'); const assert = require('assert') @@ -26,8 +26,8 @@ async function test(addon) { const { stderr, stdout } = await exec('npm install', { cwd: path.join(ADDONS_FOLDER, addon) }) - console.log(` >Runting test for: '${addon}'`); - // Disabled the checks on stderr and stdout because of this issuue on npm: + console.log(` >Running test for: '${addon}'`); + // Disabled the checks on stderr and stdout because of this issue on npm: // Stop using process.umask(): https://github.com/npm/cli/issues/1103 // We should enable the following checks again after the resolution of // the reported issue. @@ -41,7 +41,6 @@ async function test(addon) { assert.strictEqual(binding.noexcept.echo(103), 103); } - module.exports = (async function() { await beforeAll(addons); for (const addon of addons) { From 63b43f41254d144228fa8a1a8f229391ac0b9a9b Mon Sep 17 00:00:00 2001 From: raisinten Date: Thu, 19 Nov 2020 18:09:47 +0530 Subject: [PATCH 9/9] test: fix buildType bug objectwrap_worker_thread Since worker threads inherit non-process-specific options by default, the configuration needs to be shared via workerData. PR-URL: https://github.com/nodejs/node-addon-api/pull/837 Fixes: https://github.com/nodejs/node-addon-api/issues/834 Reviewed-By: Michael Dawson --- test/objectwrap_worker_thread.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/objectwrap_worker_thread.js b/test/objectwrap_worker_thread.js index 348309976..5e5e50b7e 100644 --- a/test/objectwrap_worker_thread.js +++ b/test/objectwrap_worker_thread.js @@ -1,14 +1,15 @@ 'use strict'; -const buildType = process.config.target_defaults.default_configuration; -const { Worker, isMainThread } = require('worker_threads'); +const { Worker, isMainThread, workerData } = require('worker_threads'); if (isMainThread) { - new Worker(__filename); + const buildType = process.config.target_defaults.default_configuration; + new Worker(__filename, { workerData: buildType }); } else { const test = binding => { new binding.objectwrap.Test(); }; + const buildType = workerData; test(require(`./build/${buildType}/binding.node`)); test(require(`./build/${buildType}/binding_noexcept.node`)); }