proposal: syscall/js: stricter conversion methods #29642

myitcv · 2019-01-09T22:09:43Z

This proposal is based on work originally done for GopherJS in gopherjs/gopherjs#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

The text was updated successfully, but these errors were encountered:

neelance · 2019-01-10T10:56:51Z

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 · 2019-01-10T13:37:38Z

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 · 2019-01-11T04:05:53Z

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. 😄

justinclift · 2019-01-11T07:38:12Z

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

Yaoir · 2019-01-11T18:31:12Z

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 · 2019-01-13T13:02:34Z

~~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 · 2019-01-13T13:52:44Z

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 · 2019-01-13T19:07:57Z

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 · 2019-01-14T10:11:34Z

@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 · 2019-01-14T10:19:38Z

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 · 2019-01-14T10:25:18Z

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

So I agree, the panic approach should suffice.

myitcv · 2019-01-20T19:24:59Z

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 · 2019-01-21T10:48:41Z

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 · 2019-01-23T18:38:25Z

Definitely too late for 1.12. Sorry.

martin-juhlin · 2019-02-04T21:17:08Z

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 · 2019-02-05T10:35:54Z

@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 · 2019-03-03T17:36:59Z

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 · 2019-03-03T18:02:46Z

@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 · 2019-03-20T17:52:58Z

Do you have a report, @myitcv?

myitcv · 2019-03-21T11:48:58Z

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 · 2019-03-27T17:39:18Z

Closing per discussion with @golang/proposal-review

neelance · 2019-03-27T18:13:12Z

@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 · 2019-03-28T00:35:01Z

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

This change modifies Value.String() to use the following representations for non-string values: <undefined> <null> <boolean: true> <number: 42> <symbol> <object> <function> It avoids JavaScript conversion semantics in the Go API and lowers the risk of hidden bugs by unexpected conversions, e.g. the conversion of the number 42 to the string "42". See discussion in #29642. This is a breaking change, which are still allowed for syscall/js. The impact should be small since it only affects uses of Value.String() with non-string values, which should be uncommon. Updates #29642. Change-Id: I2c27be6e24befe8cb713031fbf66f7b6041e7148 Reviewed-on: https://go-review.googlesource.com/c/go/+/169757 Run-TryBot: Richard Musiol <neelance@gmail.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

myitcv added the arch-wasm label Jan 9, 2019

gopherbot added this to the Proposal milestone Jan 9, 2019

gopherbot added the Proposal label Jan 9, 2019

sternix mentioned this issue Jan 10, 2019

syscall/js: expected panic on String method on null and undefined values #29536

Closed

andybons closed this as completed Mar 27, 2019

dmitshur mentioned this issue Apr 29, 2019

net/http: Response.Status text include "number:" prefix in WebAssembly #31736

Closed

golang locked and limited conversation to collaborators Mar 27, 2020

gopherbot added the FrozenDueToAge label Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: syscall/js: stricter conversion methods #29642

proposal: syscall/js: stricter conversion methods #29642

myitcv commented Jan 9, 2019 •

edited

Loading

neelance commented Jan 10, 2019

myitcv commented Jan 10, 2019

justinclift commented Jan 11, 2019 •

edited

Loading

justinclift commented Jan 11, 2019

Yaoir commented Jan 11, 2019

flimzy commented Jan 13, 2019 •

edited

Loading

johanbrandhorst commented Jan 13, 2019

neelance commented Jan 13, 2019

myitcv commented Jan 14, 2019

neelance commented Jan 14, 2019

myitcv commented Jan 14, 2019

myitcv commented Jan 20, 2019

neelance commented Jan 21, 2019

andybons commented Jan 23, 2019

martin-juhlin commented Feb 4, 2019

myitcv commented Feb 5, 2019

neelance commented Mar 3, 2019

myitcv commented Mar 3, 2019

bradfitz commented Mar 20, 2019

myitcv commented Mar 21, 2019

andybons commented Mar 27, 2019

neelance commented Mar 27, 2019

gopherbot commented Mar 28, 2019

proposal: syscall/js: stricter conversion methods #29642

proposal: syscall/js: stricter conversion methods #29642

Comments

myitcv commented Jan 9, 2019 • edited Loading

Problem

Potential solution

Notes

neelance commented Jan 10, 2019

myitcv commented Jan 10, 2019

justinclift commented Jan 11, 2019 • edited Loading

justinclift commented Jan 11, 2019

Yaoir commented Jan 11, 2019

flimzy commented Jan 13, 2019 • edited Loading

johanbrandhorst commented Jan 13, 2019

neelance commented Jan 13, 2019

myitcv commented Jan 14, 2019

neelance commented Jan 14, 2019

myitcv commented Jan 14, 2019

myitcv commented Jan 20, 2019

neelance commented Jan 21, 2019

andybons commented Jan 23, 2019

martin-juhlin commented Feb 4, 2019

myitcv commented Feb 5, 2019

neelance commented Mar 3, 2019

myitcv commented Mar 3, 2019

bradfitz commented Mar 20, 2019

myitcv commented Mar 21, 2019

andybons commented Mar 27, 2019

neelance commented Mar 27, 2019

gopherbot commented Mar 28, 2019

myitcv commented Jan 9, 2019 •

edited

Loading

justinclift commented Jan 11, 2019 •

edited

Loading

flimzy commented Jan 13, 2019 •

edited

Loading