-
Notifications
You must be signed in to change notification settings - Fork 464
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
Improvements to data type test functions on Napi::Value #225
Comments
LGTM |
I'll give it a go. IsExternalOf() will be IsExternal(), internally n-api uses void* so there's no type information to go off of. I'll look at submitting a PR for that and IsTypedArrayOf. IsInstanceOf will be trickier. If I come up with a clean solution I'll submit it in a separate PR given it may require changes to ObjectWrap |
Not resolved yet. I ran into a few code structure/design issues:
All of these are solvable problems, but it boils down to if its worth bubbling up all of this functionality into the Value class or not. 50/50 |
@ebickle Thanks for the update. |
I implemented something like struct NapiWrappedTypeObject {};
template <typename T>
struct NapiWrapped {
// Checks unwrap is safe by comparing a header pointer to this object
inline static NapiWrappedTypeObject type_object;
struct TypeWrapper {
NapiWrappedTypeObject* type_ptr = &type_object;
T value;
};
// regular napi_define_class, napi_new_instance, napi_wrap wrappers, ...
static napi_status try_unwrap(napi_env env, napi_value value, T** result) {
void* raw = nullptr;
if (auto status = napi_unwrap(env, value, &raw); status != napi_ok) {
return status;
}
auto typed = static_cast<TypeWrapper*>(raw);
*result = typed->type_ptr != &type_object ? nullptr : &typed->value;
return napi_ok;
}
}; But note that the Since this reserves and associates a unique memory address with each Regarding
but when you pass a constructor function the |
Going to close this out as the work seems to have stalled out. Please let us know if that was not the right thing to do. |
The Napi::Value class contains a number of "Is" functions that tests whether the value is a particular data type. Coverage is spotty for a few scenarios, making certain types of data type tests more complex than they need to be.
I'd like to propose the following additions:
bool Napi::Value::IsTypedArrayOf<T>() const;
bool Napi::Value::IsInstanceOf<T>() const;
bool Napi::Value::IsExternalOf<T>() const;
IsTypedArrayOf
Determine whether the value is a typed array of the specified type.
Example:
value.IsTypedArray<uint8_t>().
Without this function, parameter validation of typed arrays is very complex:
Value.IsTypedArray()
TypedArray
usingValue.As()
TypedArray.TypedArrayType()
is used to determine the array type.Value.As()
is used a second time to cast to the final array type. Far too complex.IsInstanceOf
Determine whether the value is an object of the specific type.
Example:
value.IsInstanceOf<MyClass>();
Currently, determining whether a value is an instance of an 'ObjectWrap' subclass and unwrapping it is complex:
Value.IsObject()
Object
usingValue.As()
.static
Napi::FunctionReferenceconstructor
property from ObjectWrap subclass as public; note that this violates abstraction principles. Alternately, add anbool IsInstance(Napi::Value)
method to the class - but at this point we're re-implementing something node-addon-api should arguably already do for us.Object.InstanceOf(constructor);
Napi::Object
already hasInstanceOf(const Function &constructor)
but it suffers from to flaws: 1) Callers need to cast values to an object before using it and 2) a reference to the constructor is needed (private in all samples!) - the C++ type cannot be used.IsExternalOf
Determines whether the value is an external value of the specified type. Note that there is no
Napi::External
type; API consumers need to 'jump' directly from aNapi::Value
to aNapi::External<T>
. There is no way for a caller to determine the type of an external without this function.The text was updated successfully, but these errors were encountered: