-
Notifications
You must be signed in to change notification settings - Fork 462
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
Missing napi_type_tag_object for Napi::External? #1293
Comments
I can still call it but I need
which means the API works for things that are not just |
We discussed this in the 10 March Node.js Node-API meeting. The documentation for the underlying
This implies that the API is limited to objects and not externals. The team needs to clarify the semantics of this function because it may have implications on other engines that implement Node-API. |
Path forward:
|
Although I can see a usage of tagging for any javascript value (a bit like using the topmost bits of a c pointer), for externals it is a question of life of segfault. I think the underlying api should be way more strict
otherwise, how do I differentiate an external created by my library and one created elsewhere? and how do they do the same? probably a big security hole. |
Opened nodejs/node#47141 for the core portion. |
Derive class `TypeTaggable` from `Value`, and let it serve as the base class for `Object` and `External`. That way, the type tagging is implemented for both classes. Signed-off-by: Gabriel Schulhof <[email protected]> Refs: nodejs#1293
Derive class `TypeTaggable` from `Value`, and let it serve as the base class for `Object` and `External`. That way, the type tagging is implemented for both classes. Additionally, exclude deleted .js files from linting. Signed-off-by: Gabriel Schulhof <[email protected]> Refs: nodejs#1293
Derive class `TypeTaggable` from `Value`, and let it serve as the base class for `Object` and `External`. That way, the type tagging is implemented for both classes. Additionally, exclude deleted .js files from linting. Signed-off-by: Gabriel Schulhof <[email protected]> Refs: nodejs#1293
@audetto type-tagging externals and wrapped objects is certainly a good practice. The problem is that type tags came along later than the wrapped objects and externals themselves. Thus, we can no longer go back and restrict their usage, because that would break all the code out there that already uses them, potentially unrestricted. We can certainly advocate for the usage of type tags so add-on authors eventually adopt the practice of type-tagging the pointers they expose to JS. |
I full understand the constraints. |
Derive class `TypeTaggable` from `Value`, and let it serve as the base class for `Object` and `External`. That way, the type tagging is implemented for both classes. Additionally, exclude deleted .js files from linting. Signed-off-by: Gabriel Schulhof <[email protected]> Refs: #1293 PR-URL: #1298 Reviewed-By: Michael Dawson <[email protected] Reviewed-By: Chengzhong Wu <[email protected]>
Fixed this in #1298. If you think more work needs to be done, please reopen this issue! |
Derive class `TypeTaggable` from `Value`, and let it serve as the base class for `Object` and `External`. That way, the type tagging is implemented for both classes. Additionally, exclude deleted .js files from linting. Signed-off-by: Gabriel Schulhof <[email protected]> Refs: nodejs/node-addon-api#1293 PR-URL: nodejs/node-addon-api#1298 Reviewed-By: Michael Dawson <[email protected] Reviewed-By: Chengzhong Wu <[email protected]>
#1261
This PR added support for
napi_type_tag_object
, but it did so forNapi::Object
.This API is mostly useful for
Napi::External
s which do no inherit fromNapi::Object
.node-addon-api/napi.h
Lines 984 to 991 in b16c762
Should this api be moved down to
Napi::Value
?The text was updated successfully, but these errors were encountered: