Improvements to data type test functions on Napi::Value

[ebickle]

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:

1. Value.IsTypedArray()
2. Cast value to a TypedArray using Value.As()
3. Use TypedArray.TypedArrayType() is used to determine the array type.
4. Use 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:

1. Value.IsObject()
2. Cast value to an Object using Value.As().
3. Expose staticNapi::FunctionReferenceconstructor property from ObjectWrap subclass as public; note that this violates abstraction principles. Alternately, add an bool 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.
4. Call Object.InstanceOf(constructor);
5. Unwrap object.

Napi::Object already has InstanceOf(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 a Napi::Value to a Napi::External<T>. There is no way for a caller to determine the type of an external without this function.

[mhdawson]

@ebickle are these things you have time to help work on?

@jasongin any objections?

[jasongin]

LGTM

[ebickle]

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

[NickNaso]

Hi @ebickle, how is the status of this issue? Is it valid yet or all has been addressed with the PR https://github.com/nodejs/node-addon-api/pull/227?

[ebickle]

@NickNaso

Not resolved yet. I ran into a few code structure/design issues:

• An implementation of IsTypedArrayOf would depend on TypedArray.TypedArrayForPrimitiveType(). Implementing the functionality would require moving the function out and into a common location - in a way, it breaks encapsulation although you could argue the "IsX" functions on Value already do that. One alternative might be to have a static method - "static bool TypedArrayOf.IsInstance(napi::Value value). Trade-offs and pros/cons weren't clear to me.

• An implementation of IsExternalOf isn't possible at the moment without quite a bit of work. I checked into the internals of V8 and it appears as though Externals don't have any way of having type data or metadata associated with them. We'd have to add some sort of wrapper - basically rolling our own RTTI into external. It feels unsafe having the current external implementation do casts without any type information and bridging across different domains (C++, Node, V8) - that's a lower level issue.

• IsInstanceOf would be handy but at the time I last looked at this I wasn't sure how to implement it. Hadn't explored it in great detail just yet - requires some knowledge of the finer points of how C++ to Javascript classes are mapped.

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

[NickNaso]

@ebickle Thanks for the update.

[simonbuchan]

I implemented something like IsExternalOf (actually for unwrap, but the same idea) with code 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 inline static trick requires C++17. Otherwise the user has to have a NapiWrappedTypeObject NapiWrapped<Foo>::type_object; declaration in a .cxx file for each Foo.

Since this reserves and associates a unique memory address with each T, you can trust this at least as much as any other RTTI system: it would require very deliberate spoofing to break.

Regarding IsInstanceOf, the issue I ran into is that the N-API docs for references they say:

[...] to reference the constructor object [...] would not be possible with a normal handle returned as a napi_value

but when you pass a constructor function the napi_typeof changes from napi_function to napi_object and it gets rejected by napi_instanceof with a type error. I've been meaning to open this as a new error on nodejs/node, but I never got around to it, since after I found out about that I continued on to the above solution (which is possibly safer, if you called the constructor for one type on an unrelated type or something?), and I wasn't clear on who was at fault, references or instanceof.

[mhdawson]

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.