Add Value.GetPromiseState and Value.GetPromiseResult

[jeromegn]

Promises are pretty useful for modern JS.

They're probably similar to other kinds of values, except they have special properties.

https://v8docs.nodesource.com/node-9.3/d3/d8f/classv8_1_1_promise.html

It's possible to get the State of a Promise and to get at its Result (once it's resolved.)

To let v8 know it should run the promises, we'd also need to call RunMicroTasks on the Isolate.

[augustoroman]

Microtasks are already being executed (see b439134).

It's already possible to get the result of a promise (or catch a promise error) using a callback, but it's difficult.

Also, unhandled promise rejections are not captured. In particular, this means than an error raised during a microtask (e.g. an error is thrown while processing .then in a promise and somebody forgot to .catch that promise) would be silently dropped. (See #21)

Getting the state and result of a promise would be extending the Value API.

I'm thinking it might make sense to extend the value API even further and expose basic types, such as:

• // Undefined, Null, Number, String, Boolean, Promise, Function, Array, Object, ...?`
  func (v *Value) Type() ValueType
  

• // if Value is a number, otherwise 0`
  func (v *Value) Number() float64
  

• // if Value is a boolean, otherwise false`
  func (v *Value) Bool() boolean
  

• // Pending/Fulfilled/Rejected if Value is a Promise, otherwise Rejected (or Invalid?)`
  func (v *Value) GetPromiseState() PromiseState
  

• // If Value is a Promise and state == Fulfilled, otherwise nil (Undefined?)`
  func (v *Value) GetPromiseResult() *Value
  

I don't really want to blow up the API and have every possible Value conversion, but these seem particularly useful -- in the tests I often use .String() and then parse the result, which is ugly. This would also be along the lines of your request (#18) for other kinds of buffers.

[jeromegn]

Oh yes, I should've updated this issue. I did notice microtasks were ran. Not entirely sure when or how though. I was almost sure the v8 embedder had to call the RunMicroTasks function.

Those functions you describe seem about right. I already have PromiseState() and PromiseResult() locally. Except my versions return an error if they're not promises.

I think Float64() is probably more idiomatic Go than Number(). I also implemented that locally, and it also returns an error if the value was not a Number. 0 could be the actual Number value and you wouldn't know if the user passed something else entirely.

There's also the possibility of using an interface to describe a Value instead of using a struct. For instance, you could have v8.Promise and v8.Number which both implement v8.Value. That interface would be dead simple really. It could just have 2 functions like String() and Value(). The latter would return the actual type of value. Most of this would be done in C++ to avoid back and forth (a big if conditional checking all v8::Value.IsX()). Of course then you'd have to reflect on the type to figure out which kind of value it is, but that's fairly cheap compared going through cgo.

[augustoroman]

I think the default MicrotasksPolicy is kAuto: https://v8docs.nodesource.com/node-9.3/d5/dda/classv8_1_1_isolate.html#a15def2a9c11f7c8b5d55ff647ccd51ea and https://v8docs.nodesource.com/node-9.3/d2/dc3/namespacev8.html#af774e70316f1ab21a93eaab1c57316a8.

The versions returning an error are fine, although a bit more cumbersome to use, especially if it's possible to pre-check the type explicitly.

I'm hesitant to add types for v8.Promise and v8.Number since that will lead to an explosion of types in the package, but if it can be done without a lot of C++ coordination then it may be the right thing.

[jeromegn]

The versions returning an error are fine, although a bit more cumbersome to use, especially if it's possible to pre-check the type explicitly.

We can check types explicitly, but it's a often a call to C, which is slow for such a simple operation.

I feel like there are probably various use cases here. People who want to run JS with go in a deterministic manner (they control the JS code) and people who need to run arbitrary, unknown, user-provided code. For the first case, I would be more ok not being sure if the value returned is the default or the actual value from the JS. For the second case (my case), I'd want to know precisely which value was passed.

I don't mind adding more code to handle errors if it provides correctness in return. It can also often be DRYed up with a generic function to handle values and errors. You can also ignore the err return value entirely and get the same behaviour: f, _ := value.Float64().

I'm hesitant to add types for v8.Promise and v8.Number since that will lead to an explosion of types in the package

It definitely adds complexity, but I think that might be the right way. When you say "C++ coordination", do you mean lots of back and forth? It'd be nice to allow various ways of using this package like:

• You may or may not want to cast the values, not casting will leave you with a generic v8::Value on which you can do the current set of operations and a bit more. For instance, you can do Float64() or whatever on it which would return a float64, error. You could also be fearless and maybe use a MustFloat64() that will panic if it errors.

• You could use a func (v v8.Value) Cast() interface{} and then reflect on it with a select or something to get the correct type. The underlying implementation does a single call to C to get the type of value this is. An even more performant version would be CastPromise() (*v8.Promise, error) in which case you're expecting a Promise, but also want to handle when it's not a Promise.

• If you don't cast, you can just pass that value around in a performant manner.

Again, just thinking out loud here :) I'm happy to do the work and submit a PR if we agree on a solution.

[jeromegn]

I've opened a PR #24 adding GetPromiseResult().

I've also been dabbling with types and getting them for values from C / v8.

A normal Get() (not getting the kind), on my computers, benchmarks:

BenchmarkGetValue-4   	  300000	      6520 ns/op	      16 B/op	       1 allocs/op

Which is pretty decent. I think it could be made faster by doing strings differently between C and go.

Getting a Value with its Kind has similar performance. No statistically significant difference:

BenchmarkGetValueWithKind-4   	  300000	      6479 ns/op	      16 B/op	       1 allocs/op

And just getting the Kind:

BenchmarkValueKind-4   	 1000000	      1573 ns/op	       0 B/op	       0 allocs/op

The C for determining the Kind is pretty beefy and ugly, I'm sure there are ways to optimize it (like making sure the most popular kinds are at the beginning of the function):

ValueKind v8_Value_KindFromLocal(v8::Local<v8::Value> value) {
  if (value->IsUndefined()) {
    return ValueKind::kUndefined;
  }
 
  if (value->IsNull()) {
    return ValueKind::kNull;
  }
  
  if (value->IsString()){
    return ValueKind::kString;
  }
  
  if (value->IsSymbol()) {
    return ValueKind::kSymbol;
  }
  
  if (value->IsArray()) {
    return ValueKind::kArray;
  }
  
  if (value->IsBoolean()) {
    return ValueKind::kBoolean;
  }
  
  if (value->IsNumber()) {
    return ValueKind::kNumber;
  }

  //...

  return ValueKind::kUnknown;
}

ValueKind v8_Value_Kind(ContextPtr ctxptr, PersistentValuePtr valueptr) {
  VALUE_SCOPE(ctxptr);

  v8::Local<v8::Value> value = static_cast<Value*>(valueptr)->Get(isolate);

  return v8_Value_KindFromLocal(value);
}

I also added a bunch of constants for all these kinds in the package:

// Value kinds
const (
	KindUnknown                   = C.kUnknown
	KindUndefined                 = C.kUndefined
	KindNull                      = C.kNull
	KindString                    = C.kString
	KindSymbol                    = C.kSymbol
	KindFunction                  = C.kFunction
	KindArray                     = C.kArray
	KindObject                    = C.kObject
	KindBoolean                   = C.kBoolean
	KindNumber                    = C.kNumber
)

Given there isn't much if an impact (if any) in inspecting the "kind", it might make sense to always get it. It gives us a way more information about the values we get and would help a lot with determining what methods can and cannot be called.

[augustoroman]

It might be worth returning a big struct from C that has the answers to all of the IsX() queries, since I think that multiple may be true, e.g. Object and Function, Object and Array, Object and Promise (ok, is it just Object + *?)

But yes, I agree that returning this alongside other results is a good idea since the overhead of making N queries in c++ is small compared to making even one more cgo jump.

[jeromegn]

Oh yes, that's a good idea.

I was puzzled while I was writing a test for the kinds, some of them kept returning "Object" since that function I made returned very early if the Value was any kind of Object.

func TestValueKind(t *testing.T) {
	ctx := NewIsolate().NewContext()
	toTest := map[string]int{
		`undefined`:                        KindUndefined,
		`null`:                             KindNull,
		`"test"`:                           KindString,
		`Symbol("test")`:                   KindSymbol,
		`(function(){})`:                   KindFunction,
		`[]`:                               KindArray,
		`new Object()`:                     KindObject,
		`true`:                             KindBoolean,
		`false`:                            KindBoolean,
		`1`:                                KindNumber,
		`new Date()`:                       KindDate,
		`(function(){return arguments})()`: KindArgumentsObject,
		`new Boolean`:                      KindBooleanObject,
		`new Number`:                       KindNumberObject,
		`new String`:                       KindStringObject,
		`new Object(Symbol("test"))`:       KindSymbolObject,
		`/regexp/`:                         KindRegExp,
		`new Promise((res, rjt)=>{})`:      KindPromise,
		`new Map()`:                        KindMap,
		`new Set()`:                        KindSet,
		`new ArrayBuffer(0)`:               KindArrayBuffer,
		`new Uint8Array(0)`:                KindUint8Array,
		`new Uint8ClampedArray(0)`:         KindUint8ClampedArray,
		`new Int8Array(0)`:                 KindInt8Array,
		`new Uint16Array(0)`:               KindUint16Array,
		`new Int16Array(0)`:                KindInt16Array,
		`new Uint32Array(0)`:               KindUint32Array,
		`new Int32Array(0)`:                KindInt32Array,
		`new Float32Array(0)`:              KindFloat32Array,
		`new Float64Array(0)`:              KindFloat64Array,
		`new DataView(new ArrayBuffer(0))`: KindDataView,
		`new SharedArrayBuffer(0)`:         KindSharedArrayBuffer,
		`new Proxy({}, {})`:                KindProxy,
		`new WeakMap`:                      KindWeakMap,
		`new WeakSet`:                      KindWeakSet,
		`(async function(){})`:             KindAsyncFunction,
		`(function* (){})`:                 KindGeneratorFunction,
		`function* gen(){}; gen()`:         KindGeneratorObject,
		// ``: KindExternal,
		// ``: KindInt32,
		// ``: KindUint32,
		// ``: KindNativeError,
		// `new Map()[@@iterator]()`: KindMapIterator,
		// `new Set()[@@iterator]()`: KindSetIterator,
		// ``: KindWebAssemblyCompiledModule,
		// ``: KindUnknown,
	}

	for script, kind := range toTest {
		if k := getKind(t, ctx, script); k != kind {
			t.Errorf("Expected kind: %d, got: %d", kind, k)
		}
	}
}

I'll play with a bigger struct with a bunch of bools. I wonder if that'll make a performance difference.

[jeromegn]

Adding a big struct of bools slows it down by quite a bit:

BenchmarkGetValue-4   	  200000	     10016 ns/op	      64 B/op	       1 allocs/op

This is probably too much:

typedef struct {
    bool isUndefined;
    bool isNull;
    bool isString;
    bool isSymbol;
    bool isFunction;
    bool isArray;
    bool isObject;
    bool isBoolean;
    bool isNumber;
    bool isExternal;
    bool isInt32;
    bool isUint32;
    bool isDate;
    bool isArgumentsObject;
    bool isBooleanObject;
    bool isNumberObject;
    bool isStringObject;
    bool isSymbolObject;
    bool isNativeError;
    bool isRegExp;
    bool isAsyncFunction;
    bool isGeneratorFunction;
    bool isGeneratorObject;
    bool isPromise;
    bool isMap;
    bool isSet;
    bool isMapIterator;
    bool isSetIterator;
    bool isWeakMap;
    bool isWeakSet;
    bool isArrayBuffer;
    bool isArrayBufferView;
    bool isTypedArray;
    bool isUint8Array;
    bool isUint8ClampedArray;
    bool isInt8Array;
    bool isUint16Array;
    bool isInt16Array;
    bool isUint32Array;
    bool isInt32Array;
    bool isFloat32Array;
    bool isFloat64Array;
    bool isDataView;
    bool isSharedArrayBuffer;
    bool isProxy;
    bool isWebAssemblyCompiledModule;
} ValueKinds;

We could also send a slice of ints for all the value's kinds. That would be more compact.

[augustoroman]

interesting! Many of those are mutually exclusive, but I'm surprised that it slows it down so much.

Also, be a little careful of performance differences, since the scope macros probably incur a bunch of overhead and I don't really understand if everything they do is 100% necessary.

[augustoroman]

Also, the go <-> c API can change as necessary as long as we don't directly export the returned values.

Perhaps you're already thinking this, but just to be clear: We probably don't want to expose the result directly, but instead cache the returned kind information (e.g. until some more JS is executed) and use that for quick type-casting or type-querying on the Go side.

[augustoroman]

Also, as a reference, I was previously under the impression that everything was an Object in JS, so I ran this test in node, which was a little maddening:

$ node
> num=5
5
> num.foo = 'abc'
'abc'
> num.foo
undefined
> num
5
> typeof num
'number'
> num.constructor.name
'Number'
> num2 = new Number(5)
[Number: 5]
> num2.foo = 'xyz'
'xyz'
> num2.foo
'xyz'
> num2
{ [Number: 5] foo: 'xyz' }
> typeof num2
'object'
> num2.constructor.name
'Number'
> num2 instanceof Number
true
> num instanceof Number
false
> num.constructor.name
'Number'
> arr = [1,2,3]
[ 1, 2, 3 ]
> typeof arr
'object'
> arr.constructor.name
'Array'
> arr
[ 1, 2, 3 ]
> arr.foo = 'bar'
'bar'
> arr
[ 1, 2, 3, foo: 'bar' ]
> typeof arr
'object'
> arr instanceof Array
true
> arr instanceof Object
true
> ob = { foo: 'bar' }
{ foo: 'bar' }
> typeof ob
'object'
> ob.constructor.name
'Object'
> ob instanceof Object
true
> fn = function() { };
[Function: fn]
> typeof fn
'function'
> fn instanceof Object
true
> num instanceof Object
false
>

Apparently numbers are not Objects.

[jeromegn]

I'm having trouble with the format for representing "kinds".

A slice of ints should work, but it's annoying to iterate through to find a value. We'd want to provide IsX() methods to make that easier.

I'm having issues getting my slice of ints to work correctly. I don't want to point to the slice of memory from C, that seems unsafe. I'm trying to copy the bytes using GoBytes + reading ints from the slice. I'm getting the wrong ints, I must be doing something wrong!

typedef enum {
	kUndefined,
	kNull,
	kString,
	kSymbol,
	kFunction,
// ...
} ValueKind;

typedef struct {
    const int* kinds;
    size_t len;
} ValueKinds;


ValueKinds v8_Value_KindsFromLocal(v8::Local<v8::Value> value) {
  std::vector<int> kinds;

  if (value->IsUndefined()) {
    kinds.push_back(ValueKind::kUndefined);
  }
 
  if (value->IsNull()) {
    kinds.push_back(ValueKind::kNull);
  }
  
  if (value->IsString()){
    kinds.push_back(ValueKind::kString);
  }
  
  if (value->IsSymbol()) {
    kinds.push_back(ValueKind::kSymbol);
  }
  
  if (value->IsArray()) {
    kinds.push_back(ValueKind::kArray);
  }
  
// ...

  return ValueKinds{kinds.data(), kinds.size()};
}

func (v *Value) Kinds() []int {
	src := C.v8_Value_Kinds(v.ctx.ptr, v.ptr)
	b := bytes.NewBuffer(C.GoBytes(unsafe.Pointer(src.kinds), C.int(src.len*SIZEOF_INT32)))
	kinds := make([]int, int(src.len))

	for i := range kinds {
		readI, _ := binary.ReadVarint(b)
		kinds[i] = int(readI)
	}
	return kinds
}

I also tried reading LittleEndian byte sizes from the buffer, got a different issue (different numbers, but still not right.)

I'm not so great at C/C++, I must be doing something wrong.

[augustoroman]

The problem is that the std::vector is cleaning up after itself when the function returns, so it's releasing the allocated data that you're pointing to, which is subsequently destroyed by any future functions (e.g. cgo stuff). You'll need to actually malloc the memory and copy into it. I'll push a PR up shortly. You shouldn't have to use the binary encoding, you should be able to read the ints directly from C.

[augustoroman]

sorry, I won't get to this tonight. I sent you an invitation for access to this repo if you'll be working on it. That way you can push up to branches and we can more directly collaborate on the same code, sharing commits. If you push up your existing code, I can fix the c <--> go communication.

[jeromegn]

The struct field has a const int* assigned to it. Isn't that a pointer to a slice? I thought I had to convert to golang bytes and then copy them over. The pointer must be unsafe at that point, I see what you mean.

I imagine I could use a int[] instead? Not sure how to get to the values from golang.

[augustoroman]

This should work for getting the values in go:

kinds_slice := (*[1 << 20]C.int)(unsafe.Pointer(src.kinds))[:src.len:src.len]

(see https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices)

However, you cannot use the pointer from kinds.data() after the kinds vector has been destructed, regardless of whether it's a const pointer or not.

You'll have to do something like:

data = calloc(kinds.size(), sizeof(int));
memcpy(data, kinds.data(), kinds.size() * sizeof(int));
return ValueKinds{data, kinds.size()};

and then, on the go side:

defer C.free(src.kinds)

[jeromegn]

Oh right, should've posted: I finally figured that out heh. Looks like what I put in code. Now I'm trying to allocate less per run (comparing the benchmarks)

Thanks!

[jeromegn]

I've made lots of progress in my free time working on this. You can compare to see how it looks with getting the kind of value: master...superfly:kind

I switched most methods to use the new ValueTuple (not in love with the name) which has the Kind into. Unfortunately, I think there's an issue when C returns early with an empty ValueKinds struct. I tried to make it work like the String struct you have and check for a nil pointer, but it still bombs. If you run the TestErrorRunningInvalidJs test, you get:

Received signal 11 SEGV_MAPERR 000000000018

==== C stack trace ===============================

[end of stack trace]

Any thoughts? I tried passing a nullptr, but I get an odd C++ errors. It only works if I pass in an empty struct for it.

In other news, testing for all kinds work. I'm happy with the results, but there's probably some work required to make the API nicer.

Edit: Ok, I figured why I couldn't pass a nullptr, I had to pass a complete struct or else it wouldn't be padded correctly. I still get those SEGV_MAPERR issues.

[jeromegn]

Alright, I figured out that getting to the errors first fixes this SEGV_MAPERR issue.

I pushed a lot of clean-ups. I like where it's going :) master...superfly:kind

However, now I get new issues on linux apparently. All tests pass fine on my mac, but not on travis: https://travis-ci.org/superfly/v8/jobs/373225829

I wonder if the way I'm casting that slice is "too unsafe".

[augustoroman]

You can't use the slice directly, because you've free'd the underlying memory after you return from the function. That memory is available to the OS and the process for any use, so the fact that it was passing on your mac was you getting lucky. I've fixed the bug and made a variety of other changes at https://github.com/augustoroman/v8/tree/kind -- feel free to push additional changes.

One thing that was weird: The WebAssembly.compile promise was always Pending on my system -- I can't figure out why it's not Resolved, so the Kinds test would fail when I used that. If I use synchronous compilation via WebAssembly.Module then it works fine. That really bothers me -- I can't explain why it's different. I'm using V8 v6.7.77, so maybe the particular version makes a difference?

[jeromegn]

Very nice cleanup. In a previous iteration, I was copying the slice over and forgot that changing this would break my logic. Is it an expensive operation? Maybe we can just add the freeing as a finalizer?

Getting a value now takes about 50% more time (ns/op). I don't know how much we care given that the original (~6K) was already "slow". I'm sure we can improve it a lot by tweaking things here and there.

Do you think this is a good change overall? It would've been nice, as you mentioned before, to return relevant data per-type, such as a promise's state. I'm not sure how we'd model that. Maybe an array of key => value pairs we pass around. That would probably add to the overhead.

One thing that was weird: The WebAssembly.compile promise was always Pending on my system -- I can't figure out why it's not Resolved, so the Kinds test would fail when I used that. If I use synchronous compilation via WebAssembly.Module then it works fine. That really bothers me -- I can't explain why it's different. I'm using V8 v6.7.77, so maybe the particular version makes a difference?

Can you try a quick implementation of RunMicroTasks and run it before trying to get at the results to see if it makes a difference? (probably will.) It wouldn't surprise me that microtasks are ran at a different point in time in later versions. I wish libv8 had way more versions we can test, and more recent ones too. Anything before 6.3 breaks because of the CXX includes flags (iirc.)

[augustoroman]

I'm ok with the slowdown in getting a value if we're happy with the exported go API -- we can optimize later as long as we're sure that we have a good API.

I'm still thinking about the API, though. Give me just a couple more days to let it sink in.

I tried RunMicrotasks, but no luck even when I sprinkled it all over the C code, running it before and after multiple calls.

I think I'm going to setup docker builds of multiple v8 versions and I'll try using that to build on multiple versions on travis.

[jeromegn]

You mean the internal API? The API for users hasn't changed.

I think I'm going to setup docker builds of multiple v8 versions and I'll try using that to build on multiple versions on travis.

I was just thinking it might be worthwhile to start building our own pre-compiled v8. Could be done automatically via travis, from a different repo. Docker seems like a good idea, the image layers could be "cached" by pushing a "base" image somewhere.

[augustoroman]

The user API is the addition of Value.IsKind and Value.GetPromiseResult, as well as the Kind* definitions. Several of the tests use Value.kinds to validate the type of the value which implies that that may be a useful operation, but I'm really hesitant to export that.

Also, the current code assumes that the kind of a value can never change. I think I've convinced myself that that is accurate, but JS is pretty squirrely. You can assign a new value to a variable, but I don't think you can change the kind that a particular value is.

[jeromegn]

I don't think getting a list of "kinds" is that useful for users. It might though, hard to tell at this point. It might ok not exporting a IsKind() method and instead recreate the various IsX().

As for GetPromiseResult(), I'm not a huge fan of the Get prefix in general. I think it's more idiomatic go to omit them and, for example, use PromiseResult().

Also, the current code assumes that the kind of a value can never change. I think I've convinced myself that that is accurate, but JS is pretty squirrely. You can assign a new value to a variable, but I don't think you can change the kind that a particular value is.

I think values won't change for the same scope. If you change a variable defined outside of a scope, it won't propagate into "children" scopes (closures.) If you reassign a variable to something else, then that's just a new value, your value is still valid until it goes out of scope.

[jeromegn]

On second thought, using an interface like:

type Value interface {
  String() string
  ToUnsafe() unsafe.Pointer
}

Might be enough.

Then we could have the proper types for each kind like Promise. They could all embed a "base" type which handles common stuff to fulfill the interface.

Then users could use reflection to figure out which kind of value they got from v8.

I think this would be the most idiomatic go way of doing it.

[augustoroman]

Wait, what? ToUnsafe? I'm confused about where this is coming from.

Having separate types for each possible Value type may make sense, although a lot of the value types won't have any special functionality -- at least, it feels like it's difficult to gradually add such functionality. If it's a MapIterator kind but we haven't written a v8.MapIteratorValue go type yet, what actual type is it? just an Object? But then code changes when we update it.

[augustoroman]

Also, if a value is multiple kinds, what underlying implementation does it get? I guess we could define a bunch of interfaces (e.g. v8.Promise as an interface) and we'll pick and choose the underlying (unexported) types based on the kind so that it supports the desired set of interfaces.

This frees us from having to export specific types that users may depend on. For example, some JS value may additionally support the new v8.MapIterator interface in the future.

Changing Value to an interface would be a major API change, but I guess we can do that if it provides a substantial benefit.

[jeromegn]

Sorry, I was talking pseudo-code when I had not started coding.

I made a quick pass and it mostly works. I still need to work on the reflection bits (the various context value creation functions), but most tests pass.

Every structs would embed the base struct Value. In my implementation, using Call on a value is only possible if it's a Function, which implements FunctionIface. ctx.Bind() specifically returns a FunctionIface.

Everything is very much a work in progress. The names ending with -Iface are mostly while I'm refactoring code.

I branched off of kid here: https://github.com/augustoroman/v8/compare/ifaces

Edit: I have now fixed the failing tests.

[jeromegn]

I think users would want to check if an interface is implemented on something doing:

obj, ok := v.(v8.ObjectIface)
if !ok {
  // stop or do something else
}
got, err := obj.Get("field")

In general, I think we'd want to keep "generic" values (v8.Value) around. We could give it back all the methods I removed from it in the branch just so people can keep using the lib as before for types we haven't yet implemented.

For multiple kinds (all non-primitive values), the most specific kind "wins" if implemented in this package. It should support the behaviour of less-specific kinds too by embedding the next-most-specific type for its kinds union (ie: Uint8Array -> TypedArray -> ArrayBufferView -> ... so Uint8Array would embed a TypedArray type if we had one.)

People can either reflect to figure out if it implements an interface, they can check for a specific type too. All without going back and forth with C++.

This is all just still a proposal. I wrote code because that's the best way I know to explain myself. Hopefully it's clearer in code than in my comments :)

[augustoroman]

No problem. I'll take a closer look tonight... unfortunately, I don't think that godoc allows me to quickly look at the branch docs, but I can do that locally when I get home.

[jeromegn]

I haven't written much docs, just some proof-of-concept code.

[augustoroman]

I like it a lot. I went through and made the underlying implementations un-exported and renamed the interfaces to the short versions, so v8.Value, v8.Array and v8.Function are all interfaces, but v8.value, v8.array, and v8.function are the actual implementations. Keeping the original v8.Value structure doesn't help compatibility since the calls to Eval and Create return the interface. That has to be the case if we go with this approach since otherwise we wouldn't get the type switching that we want.

This approach is really nice -- it's idiomatic and easy for us to extend to cover new types, and it keeps the API pretty clean.

I was even able to make the somewhat complicated scenario of the Number object work: new Number(5) in js returns a value that is both a Number and an Object, even though the bare number 5 is only a Number kind. I had to have two underlying (mostly redundant) implementations, but it works and isn't terrible, I think.

I'm trying to think of drawbacks of this approach, and so far I've come up with:

• if we mess up and assign the wrong underlying wrapper object to a value, the functionality that should be accessible is lost without a patch.
• the above applies if v8 changes or is enhanced to add functionality that partially overlaps with what we have. E.g. if a Promise suddenly becomes an ArrayBuffer or something, then we screw up the returned value.
• similarly, IsKind would be potentially inconsistent with the type switching.
• existing code is broken and must be migrated. Before we'd merge this branch I'd tag the code as version 1.0 and maybe add a vgo.mod or something and then move to version 2.

[jeromegn]

I'm glad you like it!

• if we mess up and assign the wrong underlying wrapper object to a value, the functionality that should be accessible is lost without a patch.

That's true of most code :) I wouldn't worry about it. We can write tests that would catch most potential issues, already the ones I have for the various kinds are pretty thorough.

• the above applies if v8 changes or is enhanced to add functionality that partially overlaps with what we have. E.g. if a Promise suddenly becomes an ArrayBuffer or something, then we screw up the returned value.

I've been thinking we should probably start adding conditionals depending on the v8 version. I mean, we probably don't have to just yet, but it'll be useful in the future.

• existing code is broken and must be migrated. Before we'd merge this branch I'd tag the code as version 1.0 and maybe add a vgo.mod or something and then move to version 2.

Yes, I agree. This is a breaking change and warrants a major version bump.

---

A few misc thoughts:

• I'm bringing back Kinds() as getKinds() on the Value interface. I don't trust my new tests, the previous ones were much more accurate. Your changes allowed to bring this back 👍
• I bet we can do a lot with struct tags. We could unmarshal v8 values into go structs by reading the tags and knowing which kinds of values to expect on the C++ side.

[augustoroman]

if we mess up and assign the wrong underlying wrapper object to a value, the functionality that should be accessible is lost without a patch.

That's true of most code :) I wouldn't worry about it. We can write tests that would catch most potential issues, already the ones I have for the various kinds are pretty thorough.

Sort of. I mean, if we were to simply expose all of the accessor methods on the existing v8.Value (for example: v8.Value.Float64() (float64, error)) that makes a call through the C++ API, then the IsKind accessor on v8.Value is enough to accomplish the same thing:

val, err := ctx.Eval(`something that returns a thing that acts like a JS number`, "test.js")
if err { ... }
num, err := val.Float64()
if err { ... }
...use num...

or

val, err := ctx.Eval(`something that returns a thing that acts like a JS number`, "test.js")
if err { ... }
if !val.IsKind(v8.KindNUmber) {
   ...err..
}
num := val.Float64()
...use num...

That works regardless of how we determine what wrapper class to use, and user code can do whatever they want with the returned object regardless of what we write or how v8 changes.

There's the same number of cgo function calls in all cases using the Kind approach.

In addition, no major version change is necessary.

---

• getKinds sounds good.
• struct tags is a great idea for bypassing the kinds type issue altogether, but it's still more complicated that just adding all of the methods to v8.Value.

[jeromegn]

Oh yea, good point. We can make it backwards compatible. People can still ignore the error if they're confident this is a Number (if they checked v.(v8.Number) beforehand or used IsKind())

I've already pushed getKinds and changed back the test.

I also pushed the kinds stuff support for the go_callback magic :) Totally forgot about that. Then when I tried using the package in a real-world scenario, my CallbackArgs args had no types. Whoops.

Oh, and some Promise stuff.

[augustoroman]

Update on this: I went through and started a branch (https://github.com/augustoroman/v8/tree/kind-compat) with the backwards-compatible functions of v8.Value.Float64 and without the interfaces. I think this is the correct way to go, since the interface approach can be layered on top of this simple API. Especially given the fact that V8 (and JS in general) will coerce values generously, I think that the interface approach is too restrictive for the v8 package API.

However, I ran into a surprise: Bool, Float64, and Int64 never return an error message! They do implicit conversions. For example, the JS (() => 'xyz') (which is a KindFunction) succeeds for .Float64 but returns NaN and .Int64 returns 0.

So I think that the right thing to do is to make those functions return the value without the error return value and expect callers to check IsKind first.

As a side note, I also switched the Kind checking in C++ to use a bitmask instead of an array since we have fewer than 64 values. The bitmask stuff is all opaque in the API, so we can change that later if necessary.

I think I should finish this up tomorrow or Wed evening. Then we can commit the promise stuff, add UnhandledPromiseRejection callbacks, export the explicit memory release API, and fix the registered callback leak (though I haven't looked into how to fix that yet).

[augustoroman]

Take a look at #25 and let me know what you think.