Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: syscall/js: ValueOf should accept []js.Value and map[string]js.Value as analogs of []any and map[string]any #60335

Jorropo opened this issue May 22, 2023 · 5 comments


Copy link

Jorropo commented May 22, 2023

ValueOf right now converts js.Value as [its value].

So I naively assumed I could write this code:

// racyAwait takes in multiple promises and race them against each other using Promise.race
func racyAwait(ctx context.Context, promises ...js.Value) (sucess []js.Value, err error)
  return await(ctx, js.Global().Get("Promise").Call("race", promises))

However it doesn't, I have to write this instead:

// racyAwait takes in multiple promises and race them against each other using Promise.race
func racyAwait(ctx context.Context, promises ...js.Value) (success []js.Value, err error)
  r := make([]any, len(promises))
  for i, p := range promises {
    r[i] = p
  return await(ctx, js.Global().Get("Promise").Call("race", r))

This just looks silly, I guess it also have a performance impact but it's not like syscall/js.ValueOf is particularly fast anyway.

I propose this change to it's behaviour:

 | Go                     | JavaScript             |
 | ---------------------- | ---------------------- |
 | js.Value               | [its value]            |
 | js.Func                | function               |
 | nil                    | null                   |
 | bool                   | boolean                |
 | integers and floats    | number                 |
 | string                 | string                 |
 | []interface{}          | new array              |
+| []js.Value             | new array              |
 | map[string]interface{} | new object             |
+| map[string]js.Value    | new object             |
@gopherbot gopherbot added this to the Proposal milestone May 22, 2023
Copy link

Change mentions this issue: syscall/js: implements ValueOf for slices and maps of Value

Copy link

mvdan commented May 22, 2023

I wonder if the rules could be extended to work with any slice or map where the element type is supported, like []int or map[string]string as well. Otherwise the set of rules feel slightly arbitrary.

Copy link
Contributor Author

Jorropo commented May 22, 2023

@mvdan I think it would make sense from a UX with dealing with a dynamically typed language point of view, however this adds lots of various implications (I can think about: [][][]int, [][][3]int, reflect inside ValueOf, type other int; js.ValueOf([]other{42}) ?).
I'll create a new proposal for this later, I don't think my more trivial proposition should be blocked on this.

I don't actually think this would be good, it would be surprising to have widely different performance depending on codepath taken.
I guess a solution would be to limit this to N+1 ([]int would work but not [][]int or []map[string]any and []map[string]js.Value would work but not []map[string]int) so we can hardcode all cases in the type switch but then it doesn't feel any less arbitrary.

Copy link

Looking at how this is currently implemented, I guess this would be a pair of new cases in the switch statement that have the same source code as the ones for map[string]any and []any:

	case []Value:
		a := arrayConstructor.New(len(x))
		for i, s := range x {
			a.SetIndex(i, s)
		return a
	case map[string]Value:
		o := objectConstructor.New()
		for k, v := range x {
			o.Set(k, v)
		return o

This relies on the fact that SetIndex and Set can both accept js.Value as their second argument, which is internally implemented as another to ValueOf.

It could potentially be optimized to directly call the internal valueSetIndex and bypass the ValueOf lookup on each element, but that's presumably just an implementation detail that would have the same public API as the straightforward implementation above.

Seems like a nice combination of more convenient usage and an opportunity for a potential optimization! (I've not measured whether the optimization is significant, though.)

Copy link
Contributor Author

Jorropo commented May 22, 2023

@apparentlymart Accounting for inlining I implemented all of the optimisations you layed out:
Either I can read your future thoughts, or this is a canonical patch and we converged on the same ideas, I'll let you pick 😀.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Status: Incoming

No branches or pull requests

4 participants