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

napi_get_cb_info: how to detect instance this and prototype #13824

Closed
ealib opened this issue Jun 20, 2017 · 8 comments
Closed

napi_get_cb_info: how to detect instance this and prototype #13824

ealib opened this issue Jun 20, 2017 · 8 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@ealib
Copy link

ealib commented Jun 20, 2017

  • Version: 8.1.2
  • Platform: Windows 10
  • Subsystem: N-API

Maybe a trivial issue, but I found I can't easily distinguish from this retrieved with napi_get_cb_info inside a getter called on an instance of a wrapped class, and the same getter called on that class prototype.

const example = require('example');

let m = new example.MyClass();
console.dir(m); // ok
console.dir(m.prototype); // here getters fail

The getter code fails because napi_get_cb_info returns a thisArg value which can not be used in napi_unwrap (of course).

napi_value MyClass::GetCount(napi_env env, napi_callback_info info) {
    const size_t argc_expected = 0;
    size_t argc = argc_expected;
    napi_value es_this;
    napi_status status = napi_get_cb_info(env, info, &argc, nullptr, &es_this, nullptr);
    assert(napi_ok == status);
    MyClass* native_instance = nullptr;
    status = napi_unwrap(env, es_this, reinterpret_cast<void**>(&native_instance));
    assert(napi_ok == status); // status is napi_invalid_arg when es_this is prototype
    double count = (double) native_instance->count;
    napi_value count_value;
    status = napi_create_number(env, count, &count_value);
    assert(napi_ok == status);
    return count_value;
}

Is checking if (napi_invalid_arg == status) after calling napi_unwrap the only way to detect a getter is called on prototype?

@mscdex mscdex added the node-api Issues and PRs related to the Node-API. label Jun 20, 2017
@mscdex
Copy link
Contributor

mscdex commented Jun 20, 2017

/cc @nodejs/n-api

@mhdawson
Copy link
Member

You may be able to keep the prototype for the wrapped class and check that the this passed in has that prototype. If so then you expect napi_unwrap to work ok.

We don't have an API like is_wrapped, even if we did that might only fix some cases as you might be able to pass in the wrong type of object even if it is one that was wrapped.

@ealib
Copy link
Author

ealib commented Jul 2, 2017

@mhdawson since I save a reference to the class constructor, maybe I can use napi_instanceof to find if this is created by that very constructor:

napi_value MyClass_constructor = nullptr;
status = napi_get_reference_value(env, MyClass::es_constructor, & MyClass_constructor);
assert(napi_ok == status);
bool is_instance = false;
status = napi_instanceof(env, es_this, MyClass_constructor, &is_instance);
assert(napi_ok == status);
if (is_instance) {
    // napi_unwrap() ...
} else {
    // otherwise...
}

@digitalinfinity
Copy link
Contributor

@ealib that sounds reasonable- did that workaround work for you or do you need changes in n-api?

@jasongin
Copy link
Member

jasongin commented Sep 4, 2017

Saving a persistent reference to the class constructor for use with later instanceof checks is a very common pattern for native addons. We should probably document that.

@mhdawson
Copy link
Member

mhdawson commented Sep 6, 2017

@ealib, if that worked for you it would be great to add it to the documentation. If you are interested I could help you submit a PR ? If you don't have time I'll take a look at it.

@ealib
Copy link
Author

ealib commented Sep 28, 2017

Sorry for the late reply.

@digitalinfinity yes it works OK so far

@jasongin yes, saving a reference to constructor is a pattern I use for 100% classes because the native module I am working on exposes a few functions to lookup some entities and returned IDs are used to actually get (wrapped) instances of those very entities (another old pattern)

@mhdawson I just updated the module code to use napi_get_new_target/2, but apart that atm I don'thave time for a PR. If not urgent, I can possibly edit the doc by mid October.

@mhdawson
Copy link
Member

mhdawson commented Nov 2, 2017

@ealib, @jasongin created this PR to document. Please review if you have a chance: #16699

MylesBorins pushed a commit that referenced this issue Dec 12, 2017
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 pushed a commit that referenced this issue Dec 19, 2017
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 pushed a commit that referenced this issue Dec 20, 2017
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>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 16, 2018
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 issue Apr 16, 2018
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

5 participants