Go: proposal: syscall/js: stricter conversion methods

[myitcv]

_This proposal is based on work originally done for GopherJS in https://github.com/gopherjs/gopherjs/issues/617._

Problem

Currently, any syscall.js.Value can have String() called on it, and that is guaranteed to succeed, because JavaScript's String(thing) function can take anything as an argument (this is where it is used). For example:

func main() {
    var v js.Value

    v = eval(`global.X = undefined`)
    fmt.Printf("global.X: %v\n", v.String())

    v = eval(`global.X = null`)
    fmt.Printf("global.X: %v\n", v.String())

    v = eval(`global.X = 5`)
    fmt.Printf("global.X: %v\n", v.String())

    v = eval(`global.X = new Number(5)`)
    fmt.Printf("global.X: %v\n", v.String())

    v = eval(`global.X = [1, 2, 3]`)
    fmt.Printf("global.X: %v\n", v.String())
}

func eval(s string, args ...interface{}) js.Value {
    v := fmt.Sprintf(s, args...)
    fmt.Printf("> %v\n", v)
    return js.Global().Call("eval", v)
}

gives:

> global.X = undefined
global.X: undefined
> global.X = null
global.X: null
> global.X = 5
global.X: 5
> global.X = new Number(5)
global.X: 5
> global.X = [1, 2, 3]
global.X: 1,2,3

By contrast, the Int(), Float(), and Bool() methods are much stricter. They require a js.Value with a JavaScript type of number, number and boolean respectively. Consider the following example for Int():

func main() {
    var v js.Value

    v = eval(`global.X = 5`)
    fmt.Printf("global.X: %v\n", v.Int())

    v = eval(`global.X = new Number(5)`)
    fmt.Printf("global.X:%v\n", v.Int())
}

gives:

> global.X = 5
global.X: 5
> global.X = new Number(5)
panic: syscall/js: call of Value.Int on object
...

I believe that the strictness of Int(), Float() and Bool() methods is correct; with this strictness, I can make strong assertions about the fact that only a number, number or boolean can have been the source value. I discuss below whether panic-ing is the correct approach when the source type is incorrect.

By contrast, I believe the current implementation of String() will cause issues. Precisely because I _can't_ make any assertions about the source value: I can't opt-out of this automatic conversion that happens along the way. Instead everywhere I call String() I need to defensively code to ensure that I actually have a string value on the JavaScript side before treating it as such on the Go side:

func main() {
    var v js.Value

    v = eval(`global.X = "hello"`)
    if v.Type() == js.TypeString {
        fmt.Printf("global.X: %v\n", v.String())
    } else {
        fmt.Printf("global.X: not a string\n")
    }

    v = eval(`global.X = 5`)
    if v.Type() == js.TypeString {
        fmt.Printf("global.X: %v\n", v.String())
    } else {
        fmt.Printf("global.X: not a string\n")
    }
}

gives:

> global.X = "hello"
global.X: hello
> global.X = 5
global.X: not a string

However, the current implementation of String() is useful in one important respect (thanks to @neelance for reminding me of this): fmt. It allows a js.Value value to implement fmt.Stringer. Having the JavaScript String() representation of a value makes sense to me.

That said, we can't just change String() to be as strict as Int() et al.

Potential solution

My proposal is that we leave String() as is for fmt reasons, remove Int(), Float() and Bool(), and choose one of the following options (there could be others):

Option 1: define AsX() methods that have the same strict semantics as Int() et al currently do:

func (v Value) AsBool() bool
func (v Value) AsFloat() float64
func (v Value) AsInt() int
func (v Value) AsString() string

That is, unless a method saw the a boolean, number, number or string (respectively) JavaScript typed value, it would panic.

Option 2: same as option 1, but instead of panic-ing, return an error:

func (v Value) AsBool() (bool, error)
func (v Value) AsFloat() (float64, error)
func (v Value) AsInt() (int, error)
func (v Value) AsString() (string, error)

Option 3: switch to more of an encoding parse style:

func (v Value) AsBool(*bool) error
func (v Value) AsFloat(*float) error
func (v Value) AsInt(*int) error
func (v Value) AsString(*string) error

This would also have the added benefit of being slightly more future proof in case we decided to relax the very strict semantics of requiring a boolean, number, number or string, and instead allowed undefined and null to be interpreted as the zero value (encoding/json behaves in this way).

Notes

• The As method name prefix above is of course not set in stone
• I think the syscall/js docs should also spell out any conversions that happen along the way too. For example, when converting a string value JavaScript <-> Go we end up performing a UTF16 <-> UTF8 conversion. This point is not tied to this proposal however, I just mention it in passing given the main focus of the proposal/issue is conversion JavaScript <-> Go

cc @neelance

[neelance]

What about only adding MustString() with the strict semantics? It is a bit inconsistent, but I'm not sure that it is worth to clutter the other function names just because of this. It would also be good to stay similar to the reflect package.

@bradfitz I'd like your opinion on this if you don't mind.

[myitcv]

Thanks @neelance.

It would also be good to stay similar to the reflect package.

The encoding/json analogy seems more appropriate to my mind (hence option 3 above), not least because there is some conversion required from the JavaScript value.

What about only adding MustString() with the strict semantics?

Yes, I think an inconsistency of this sort in a brand new API would be a shame.

In a Slack discussion we also came up with To as a prefix. So, taking option 1:

func (v Value) ToBool() bool
func (v Value) ToFloat() float64
func (v Value) ToInt() int
func (v Value) ToString() string

[justinclift]

Avoiding the panic sounds important to me, just from the perspective of being able to catch when a problem occurs - using standard "if err != nil" approach - for handling.

I'd steer clear of option 1. :smile:

[justinclift]

Name wise.. the To* versions sound better than the As* versions (to me).

[Yaoir]

Out of the three options, I definitely prefer option 2. It's a little more complicated than option 1, but avoids panics. Also, compare the situation to converting strings to other types in Go. The strconv package Parse_Type_() functions like ParseInt() and ParseBool() accept a string and return an error if the string couldn't be converted to that type.

[flimzy]

I definitely prefer option 2, or a slight variation, 2b, if the only type of error is a type mismatch:

func (v Value) AsBool() (bool, bool)
func (v Value) AsFloat() (float64, bool)
func (v Value) AsInt() (int, bool)
func (v Value) AsString() (string, bool)

I'm now in favor of the panic method, as described here.

[johanbrandhorst]

Re: the bikeshed; Rust has chosen as_x for these methods:

https://docs.rs/wasm-bindgen/0.2.11/wasm_bindgen/struct.JsValue.html

[neelance]

I think we should not return an error value (or ok value), since it is functionally equivalent to a simple check of the Type() and it puts a burden on each and every use.

[myitcv]

@neelance on the question of returning an error/bool vs panic, I'm not sure it's that clear cut.

A number of JavaScript APIs allow fields of effectively union type. e.g. string | []string. For such fields you'd have to implement error handling via a defer, which works but isn't very pleasant.

To my mind, if we have to make a decision one way or the other here it should be somewhat guided by how often such a pattern would need to be handled in practice. I don't have any gut feel/numbers on that.

Alternatively, don't force a decision between the two, instead add additional API methods for the error/bool handling, effectively a hybrid of options 1 and 2.

// from option 1
func (v Value) AsBool() bool
func (v Value) AsFloat() float64
func (v Value) AsInt() int
func (v Value) AsString() string

// from option 2
func (v Value) AsBoolErr() (bool, error)
func (v Value) AsFloatErr() (float64, error)
func (v Value) AsIntErr() (int, error)
func (v Value) AsStringErr() (string, error)

[neelance]

A number of JavaScript APIs allow fields of effectively union type. e.g. string | []string. For such fields you'd have to implement error handling via a defer, which works but isn't very pleasant.

I don't understand this, please post example code.

[myitcv]

Actually, I'm wrong here. Because you'd almost certainly use Type() in such situations.

So I agree, the panic approach should suffice.

[myitcv]

Think we're waiting on @bradfitz to opine on this... Anyone else?

Would it be possible to get this in for 1.12, given that WASM is still experimental?

[neelance]

I think it is too late in the cycle to get this into 1.12. Also, we don't seem to have a consensus yet.

[andybons]

Definitely too late for 1.12. Sorry.

[martin-juhlin]

_Adding MustString() or do nothing at all is the best solution._

Using To/As indicate that a conversion of some sort is taking place. If the type is a float with value 3.1415, and I invoking ToInt(), I expect it to return 3, not panic.

We are actually getting current value. _Get_ is a better prefix as in GetBool(), GetInt() etc, but in Go we don't prefix getters with _Get_, therefor the methods should be namned Bool(), Int() etc

option 2 and 3 is also hard to use. Example a = b + c, in current form it can be written as a = b.Int()+ c.Int(). For option 2 and 3 that result in multiple lines.

[myitcv]

@martin-juhlin

Using To/As indicate that a conversion of some sort is taking place.

syscall/js is the interface between JavaScript values and Go values. For string values, for example, a conversion happens to/from UTF8/UTF16. To my mind this package should be a type-safe interface that is limited to/concerned with conversion between these two "worlds". Things like float to integer conversion should happen elsewhere and explicitly so, either in the JavaScript world before they get passed through syscall/js, or in the Go world after the value has passed through.

The problem with MustString() is that it creates an inconsistent API. If I am writing an interface to a JavaScript library/some such, then I end up using the following methods:

// option "Richard"
func (v Value) Bool() bool
func (v Value) Float() float64
func (v Value) Int() int
func (v Value) MustString() string

_despite_ there being a String() method. It just feels like having to say to people "use Int(), Bool() and Float(), but remember to use MustString() for string values" is an unnecessary caveat and a somewhat inevitable pitfall.

Instead, having a consistent API is much clearer and less susceptible to user error:

// option 1
func (v Value) AsBool() bool
func (v Value) AsFloat() float64
func (v Value) AsInt() int
func (v Value) AsString() string

[neelance]

I would like to propose an alternative, inspired by https://golang.org/pkg/reflect/#Value.String: For non-string values instead of using JS's type conversion we use the following string representations:

<undefined>
<null>
<boolean: true>
<number: 42>
<symbol: abc>
<object>
<function>

This has the following properties:

• No panic, thus compatible with fmt.Stringer
• No JS conversion semantics in the Go API
• No unexpected type conversion of the number 42 to the string "42" which may hide a bug
• No cluttered names
• No inconsistency with the reflect package

The downside of the inconsistency between String() and Int() etc. remains, but this tradeoff already has its precedence in the reflect package.

If one is processing external data and strict type checking is required, then explicit checking of the Type() is necessary anyways, since it is bad Go style to rely on recovering a panic for catching bad external input.

[myitcv]

@neelance and I discussed this further offline.

I think the conclusion we reached is that what I'm after is better suited to a package/approach that sits atop syscall/js. I'm going to experiment with my React wrapper and report back.

[bradfitz]

Do you have a report, @myitcv?

[myitcv]

Haven't had a chance yet @bradfitz. So I'd suggest we either put this on hold or close it. We can always resurrect the issue later if needs be

[andybons]

Closing per discussion with @golang/proposal-review

[neelance]

@andybons What about my proposal to change the results of String() for non-string values? It still seems like a strict improvement to me and I haven't yet seen any arguments against it. I was planning to send a CL in the next few days.

[gopherbot]

Change https://golang.org/cl/169757 mentions this issue: syscall/js: improve Value.String() for non-string values