FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: document common pattern for instanceof checks by mhdawson · Pull Request #16699 · nodejs/node · GitHub

/ node Public

doc: document common pattern for instanceof checks - #16699

Closed
mhdawson wants to merge 3 commits into
nodejs:masterfrom
mhdawson:napi-doc-ref
ConversationCommits (3)ChecksFiles changed
Closed

doc: document common pattern for instanceof checks#16699
mhdawson wants to merge 3 commits into
nodejs:masterfrom
mhdawson:napi-doc-ref

Conversation

mhdawson commented Nov 2, 2017

Copy link
Copy Markdown
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-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
mhdawson mentioned this pull request Nov 2, 2017
Closed

devsnek commented Nov 2, 2017

Copy link
Copy Markdown
Member

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

mhdawson commented Nov 3, 2017

Copy link
Copy Markdown
Member Author

@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 commented Nov 3, 2017

Copy link
Copy Markdown
Member

@mhdawson ok, sorry for the mixup 👍

cjihrig approved these changes Nov 3, 2017
View reviewed changes
Comment thread doc/api/n-api.md Outdated
status = napi_instanceof(env, es_this, MyClass_constructor, &is_instance);
assert(napi_ok == status);
if (is_instance) {
// napi_unwrap() ...

cjihrig Nov 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread doc/api/n-api.md Outdated
}
```

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

cjihrig Nov 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You can drop "Of course"

mhdawson commented Nov 8, 2017

Copy link
Copy Markdown
Member Author

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

mhdawson commented Nov 17, 2017

Copy link
Copy Markdown
Member Author

Ci run: https://ci.nodejs.org/job/node-test-pull-request/11497/

mhdawson commented Nov 17, 2017

Copy link
Copy Markdown
Member Author

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 commented Nov 17, 2017

Copy link
Copy Markdown
Member Author

Landed as 9b4ab14

mhdawson closed this Nov 17, 2017
mhdawson added a commit that referenced this pull request Nov 17, 2017
doc: document common pattern for instanceof checks
9b4ab14 
PR-URL: #16699
Fixes: #13824
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
doc: document common pattern for instanceof checks
8dc05e4 
PR-URL: #16699
Fixes: #13824
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
doc: document common pattern for instanceof checks
d738c6b 
PR-URL: #16699
Fixes: #13824
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
doc: document common pattern for instanceof checks
010a330 
PR-URL: #16699
Fixes: #13824
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Merged
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
doc: document common pattern for instanceof checks
f450931 
PR-URL: nodejs#16699
Fixes: nodejs#13824
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
doc: document common pattern for instanceof checks
35dc8ba 
Backport-PR-URL: #19447
PR-URL: #16699
Fixes: #13824
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins mentioned this pull request Apr 16, 2018
Merged
mhdawson deleted the napi-doc-ref branch September 30, 2019 13:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL