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

src: add Object::TypeTag, Object::CheckTypeTag #1261

Closed
wants to merge 4 commits into from

Conversation

KevinEady
Copy link
Contributor

Adds support for napi_type_tag_object via Object::TypeTag and napi_check_object_type_tag via Object::CheckTypeTag.

Closes: #1260

@KevinEady
Copy link
Contributor Author

KevinEady commented Jan 2, 2023

Looks like the test has different outcomes depending on Node version, which I can reproduce locally. The test (cpp, js) is similar to the one found in node core (cpp, js).

It looks like there was an issue in type tag checking nodejs/node#43786 fixed by nodejs/node#43788 ... Was this backported to other Node versions?

EDIT: I just changed the test to not use the bugged checks if node version < 18.

v14: Failure ❌

➜  node-addon-api git:(add-type-tag-methods) ✗ nvm use v14                        
Now using node v14.21.2 (npm v6.14.17)
➜  node-addon-api git:(add-type-tag-methods) ✗ node test/object/object_type_tag.js
(node:13959) UnhandledPromiseRejectionWarning: AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at test (/Users/kevineady/Documents/Projects/node-addon-api/test/object/object_type_tag.js:17:10)
    at Object.exports.runTest (/Users/kevineady/Documents/Projects/node-addon-api/test/common/index.js:122:27)
(Use `node --trace-warnings ...` to show where the warning was created)
(node:13959) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:13959) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

v16: Failure ❌

➜  node-addon-api git:(add-type-tag-methods) ✗ nvm use v16                        
Now using node v16.15.1 (npm v8.11.0)
➜  node-addon-api git:(add-type-tag-methods) ✗ node test/object/object_type_tag.js
node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:

false !== true

    at test (/Users/kevineady/Documents/Projects/node-addon-api/test/object/object_type_tag.js:17:10)
    at Object.exports.runTest (/Users/kevineady/Documents/Projects/node-addon-api/test/common/index.js:122:27) {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: false,
  expected: true,
  operator: 'strictEqual'
}

v18: Success ✅

➜  node-addon-api git:(add-type-tag-methods) ✗ nvm use v18                        
Now using node v18.12.1 (npm v8.19.2)
➜  node-addon-api git:(add-type-tag-methods) ✗ node test/object/object_type_tag.js

v19: Success ✅

➜  node-addon-api git:(add-type-tag-methods) ✗ nvm use v19    
Now using node v19.3.0 (npm v9.2.0)
➜  node-addon-api git:(add-type-tag-methods) ✗ node test/object/object_type_tag.js

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request Jan 13, 2023
PR-URL: #1261
Reviewed-By: Michael Dawson <midawson@redhat.com
@mhdawson
Copy link
Member

Landed as b11e4de

@mhdawson mhdawson closed this Jan 13, 2023
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
PR-URL: nodejs/node-addon-api#1261
Reviewed-By: Michael Dawson <midawson@redhat.com
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

Successfully merging this pull request may close these issues.

Missing napi_type_tag_object?
2 participants