Exposing Value::TypeOf

[ghost]

#957

[CLAassistant]

All committers have signed the CLA.

[bartlomieju]

Cool, can you add a test somewhere in test_api.rs comparing types of two strings (to assert that it matches) and then a string and a number?

[ghost]

I can have a go at it.

[ghost]

Excuse terminology or if my understanding is incorrect. As I would like to work through this. What I just realized this morning is that I did not bind v8__Value__TypeOf in bindings.cc. However if I bind this function in bindings.cc. I still receive an undefined reference to the function itself.

const v8::String v8__Value__TypeOf(const v8::Value& self, v8::Isolate* isolate) {
  return self.TypeOf(isolate);
}

note: /usr/bin/ld: /root/Deno/rusty_v8/target/debug/deps/test_api-f7990818cf20050a.4svfdgqfs3t2c6tr.rcgu.o: in function `v8::value::<impl v8::data::Value>::type_of':
          /root/Deno/rusty_v8/src/value.rs:709: undefined reference to `v8__Value__TypeOf'
          collect2: error: ld returned 1 exit status

  = help: some `extern` functions couldn't be found; some native libraries may need to be installed or have their path specified
}

[littledivy]

To build with changes in bindings.cc you need to set V8_FROM_SOURCE=1

V8_FROM_SOURCE=1 cargo build

[ghost]

I will continue troubleshooting later today there are tons of examples on how to test. I suppose that makes sense how can I pass an independent copy of V8 as a parameter and or expect to use to the newly implemented function if the source hasn't been compiled locally on my machine instead of using the prebuilt version using cargo build

[littledivy]

*const String - V8 String is a reference not an owned String

[ghost]

So my original commit was correct in passing the Isolate as a mutable reference?

[bartlomieju]

No - this is correct you need to pass raw pointers between Rust and C++

[ghost]

I was having some memory issues with WSL2 on Ubuntu 22.04 and 20.04 on Windows 11 while compiling on this tiny laptop. So Powershell for everything is it these days. I can atleast compile this with sccache installed within 15-20 seconds.

If I was reading the docs correctly Local<String> should be the return type on TypeOf. What am I missing here since I am stating that I want to call C directly?

v8::Local<v8::String> v8__Value__TypeOf(v8::Value& self, v8::Isolate* isolate) {
  return self.TypeOf(isolate);
}

../../../src/binding.cc(762,23): error: 'v8__Value__TypeOf' has C-linkage specified, but returns user-defined type 'v8::Local<v8::String>' which is incompatible with C [-Werror,-Wreturn-type-c-linkage]
  v8::Local<v8::String> v8__Value__TypeOf(v8::Value& self, v8::Isolate* isolate) {

[bartlomieju]

@WingZer0o take a look at other APIs that return the same type - eg. v8__StackFrame__GetScriptNameOrSourceURL. You will need to call local_to_ptr on the thing you want to return and specify type as const v8::String *. Then on Rust side you will need to map that type back to v8::Local using scope.cast_local()

[ghost]

Included my test case for this PR. Please let know if anything needs to be changed if I messed up. This was a good issue for me to work on to solidify some knowledge I had from Node.js conceptually not practically. I've always wanted to get involved in either project considering all this browser work that I do and my old TypeScript projects, have to dive in I guess.

[bartlomieju]

Included my test case for this PR. Please let know if anything needs to be changed if I messed up. This was a good issue for me to work on to solidify some knowledge I had from Node.js conceptually not practically. I've always wanted to get involved in either project considering all this browser work that I do and my old TypeScript projects, have to dive in I guess.

@WingZer0o this is a great first contribution! It looks good to me, please make sure to run cargo fmt to make CI happy on formatting. (If that doesn't work do rustfmt ./src/lib.rs and rustfmt ./tests/test_api.rs)

[ghost]

Looks like the fixed a few formatting errors.

[bartlomieju]

LGTM, thanks @WingZer0o!

addressed