Add Symbols. #194
Add Symbols. #194
Conversation
| // Supports all value types, eg: Object, Array, Date, Set, Map etc | ||
| // If the value passed is a Go supported primitive (string, int32, uint32, int64, uint64, float64, big.Int) | ||
| // then a *Value will be created and set as the value property. | ||
| func (o *Object) SetSymbol(key *Symbol, val interface{}) error { |
There was a problem hiding this comment.
It seems like this is further deviating from the V8 API. The underlying v8::Object::Set method takes a Local, so perhaps v8go.Object.Set should similarly accept any v8go.Value. Since v8go.Object.Set currently accepts a string, I think that would mean making the key an interface{} type so it could accept a string or a v8go.Valuer type value. Doing that would also allow a v8 string to be created once and used several times, rather than creating a new v8 string for each set.
If we did generalize v8go.Object.Set in that way, then perhaps it should also handle integer type values to delegate to v8go.Object.SetIdx, to avoid confusion with the fact that v8::Object::Set is overloaded to accept an index.
The would have the disadvantage of changing v8::Object::Set to accept an interface{} type would be that it wouldn't provide static type checking. @rogchap any thoughts?
There was a problem hiding this comment.
Thanks for the review.
An additional complexity is that ObjectTemplate::Set takes a Name, not a Value. String and Symbol are the two Name subclasses. As you can see, I opted not to add a NamePtr, but instead downcast a ValuePtr. The main reason for this is that the structs on the Go-side are not castable to each other (e.g. an Object should ideally be assignable to a Value). And so I opted to add a separate SetSymbol in both those places, and just punt the assignability issue. Happy to have the discussion here.
We could create tagging interfaces so that Set takes a MagicValuer, and wrappers around string and int can create MagicValuers. Ideally, we'd introduce Name, String and Symbol and change it so these are interfaces that are assignable the way they are in C++. I.e. work on a type hierarchy that keeps the type safety. But it's a major API change.
I could also imagine a MustString that simplifies getting a string literal into a Name/Value. At that point, I don't think making a Set(Value) (and Set(Name) for ObjectTemplate) is too bad in terms of call site verbosity.
There was a problem hiding this comment.
The way downcasting is done for Value types is to use the v8go.Valuer interface. Possibly the same approach could be taken for Name types (Symbol or String) with a Namer interface.
We could create tagging interfaces so that
Settakes aMagicValuer, and wrappers aroundstringandintcan createMagicValuers.
Primitive types like string and int don't have any methods that we can create a non-empty interface for.
I do think having Must functions could be useful in general, since there are definitely times when no exception would be expected from creating a v8 string. I think the main reasons it fails are from the string being too long and possibly from execution terminating, which aren't relevant to initialization of the V8 context from first-party code.
Requiring a v8 String instead of just a Go string would be more inline with V8's API and would allow us to just use the Valuer interface. However, it would certainly be an inconvenience, especially as a breaking change, so I'm not sure about making that change.
There was a problem hiding this comment.
Primitive types like string and int don't have any methods that we can create a non-empty interface for.
I mean something like
type Name interface {
isName()
}
type String string
func (String) isName() {}
type Index int
func (Index) isName() {}
func (Symbol) isName() {}Possibly with an asName as well.
It requires object.Set(String("hello"), value), but that might be acceptable verbosity, similar to using a longer function name.
I think the main reasons it fails are from the string being too long and possibly from execution terminating
I'm guessing V8 doesn't validate UTF-8. Does it?
There was a problem hiding this comment.
type String string seems like it would be quite weird to have in the v8go package, since it would end up being referred to through the package's import name (e.g. v8.String) and I would expect that type name to refer to the a v8::String wrapper.
However, I think they could just create the v8 string directly. E.g. object.Set(v8.MustNewString("hello"), value)
It does seem a bit inconsistent that some APIs coerce Go primitive values (e.g. Object.Set, Object.SetIdx and Template.Set), but v8go.Valuer is used elsewhere (e.g. JSONStringify, Function.Call, PromiseResolver.Resolve).
V8 doesn't validate UTF-8. v8::NewFromUtf8 documentation says
Only returns an empty value when length > kMaxLength.
There was a problem hiding this comment.
Agree on all accounts. Requiring more explicit Value/Name creation, but providing Must functions seems nice to me. There could be "contrib" adaptor types that help with implicit conversion for those who want it. I.e. types that does Set(string, *Value) and calls Set(MustNewString(key), value).
Only returns an empty value when length > kMaxLength.
Ah, thanks for looking that up.
This allows e.g. setting the @@iterator property on an object, turning it into an Iterable.
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
==========================================
- Coverage 95.23% 91.12% -4.12%
==========================================
Files 12 13 +1
Lines 483 541 +58
==========================================
+ Hits 460 493 +33
- Misses 14 34 +20
- Partials 9 14 +5
Continue to review full report at Codecov.
|
This allows e.g. setting the @@iterator property on an object, turning
it into an Iterable.