Skip to content

Conversation

@mhdawson
Copy link
Member

Fixes: #13824

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

doc

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. dont-land-on-v4.x node-api Issues and PRs related to the Node-API. labels Nov 2, 2017
@devsnek
Copy link
Member

can you also document Napi::Object::InstanceOf usage

@mhdawson
Copy link
MemberAuthor

@devsnek I believe Napi::Object::InstanceOf is part of the wrapper not the core N-API. It should be documented here: https://github.com/nodejs/node-addon-api/blob/master/doc/object.md. It is currently empty but there is a push to add documentation there.

@devsnek
Copy link
Member

@mhdawson ok, sorry for the mixup 👍

status = napi_instanceof(env, es_this, MyClass_constructor, &is_instance);
assert(napi_ok == status);
if (is_instance){
// napi_unwrap() ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Should indentation be two spaces here and a couple lines down?

}
```

Of course the reference must be freed once it is no longer needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop "Of course"

@mhdawson
Copy link
MemberAuthor

@cjihrig thanks for the comments will update to fix and then land when I'm back from vacation in a week.

@mhdawson
Copy link
MemberAuthor

@mhdawson
Copy link
MemberAuthor

Failure on Fedora looks unrelated

Building addon /home/iojs/build/workspace/node-test-commit-linux/nodes/fedora24/test/addons-napi/7_factory_wrap/ env: ‘./node’: Permission denied Makefile:338: recipe for target 'test/addons-napi/.buildstamp' failed make[1]: *** [test/addons-napi/.buildstamp] Error 1 make[1]: *** Waiting for unfinished jobs.... 

Given that this is a doc change.

@mhdawson
Copy link
MemberAuthor

Landed as 9b4ab14

mhdawson added a commit that referenced this pull request Nov 17, 2017
PR-URL: #16699Fixes: #13824 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
PR-URL: #16699Fixes: #13824 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Dec 12, 2017
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
PR-URL: #16699Fixes: #13824 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
PR-URL: #16699Fixes: #13824 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@gibfahngibfahn mentioned this pull request Dec 20, 2017
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
PR-URL: nodejs#16699Fixes: nodejs#13824 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Backport-PR-URL: #19447 PR-URL: #16699Fixes: #13824 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Apr 16, 2018
@mhdawsonmhdawson deleted the napi-doc-ref branch September 30, 2019 13:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docIssues and PRs related to the documentations.node-apiIssues and PRs related to the Node-API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@mhdawson@devsnek@jasnell@lpinca@cjihrig@gibfahn@nodejs-github-bot