dominikh / go-js-dom

MIT License
252 stars 42 forks source link

No nice way to set CanvasRenderingContext2D.FillStyle to gradient, pattern. #45

Open dmitshur opened 7 years ago

dmitshur commented 7 years ago

This is an API design issue.

The fillStyle property of CanvasRenderingContext2D interface can be assigned any of 3 different types, according to https://developer.mozilla.org/en-US/docs/Web/API/CanvasRenderingContext2D/fillStyle:

image

However, CanvasRenderingContext2D.FillStyle field is defined as a string:

https://github.com/dominikh/go-js-dom/blob/790c7894c96d07ad1d2081aca7091a894de79a46/dom.go#L1915

So the following code will fail to compile:

gradient := ctx.CreateRadialGradient(0, 0, 8*1.75, 0, 0, 0)
gradient.AddColorStop(0, "rgba(0, 0, 0, 0)")
gradient.AddColorStop(1, "rgba(0, 0, 0, 0.3)")
ctx.FillStyle = gradient
cannot use gradient (variable of type *honnef.co/go/js/dom.CanvasGradient) as string value in assignment

I can think of some solutions, but I'm not sure which one we should pick. They all have trade-offs.

Possible solution 1

Don't do anything special. Let users do use *js.Object API:

ctx.Set("fillStyle", gradient)

This works, but perhaps it's not as nice as one could wish for.

(I'm using this as my current workaround.)

Possible solution 2

One potential solution is to change FillStyle type to interface{}:

FillStyle interface{} `js:"fillStyle"` // Can be assigned one of string, *CanvasGradient, *CanvasPattern.

Then all these are possible:

ctx.FillStyle = "rgb(255, 0, 0)"
ctx.FillStyle = gradient
ctx.FillStyle = pattern

This works, but interface{} isn't great for APIs. Any other type can be assigned too, and when querying the value of FillStyle, you get an interface{} which isn't nice either.

Possible solution 3

Define 3 fields of 3 different types, all mapping to JS property fillStyle:

FillStyle         string          `js:"fillStyle"`
FillStyleGradient *CanvasGradient `js:"fillStyle"`
FillStylePattern  *CanvasPattern  `js:"fillStyle"`

Then all these are possible:

ctx.FillStyle = "rgb(255, 0, 0)"
ctx.FillStyleGradient = gradient
ctx.FillStylePattern = pattern

This works, but now there are 3 fields for one field... Which may be confusing and creates bloat.

/cc @luckcolors FYI, in case you have ideas on this.

luckcolors commented 7 years ago

I would suggest we either use option 2 or 3.

Option 2 would be okay only if you document what should go into the interface, i've seen some packages wich don't do that and leave what should go into the interface to the imagination of who uses them.

Option 3 would be more verbose and everyone will immeditely be able to see the function signature directly from the ide: I used this solution for the functions DrawImage DrawImageWithDst DrawImageWithSrcAndDst. In that case the only difference was just the number of arguments, but i don't think having more than version of the same function is an issue, is probably still better than to use reflection with the interfaces.

A combination of 2 and 3 could also be done, so both having a generic version of the function wich just takes an interface and the typed versions like FillStyleWithColor FillStyleWithGradient FillStyleWithPattern .

At then end of the day there's not really anything better we can do.

dominikh commented 7 years ago

I oppose option 3. It's akin to unions, a data type that is not available in traditional Go, and for a good reason.

In particular, I don't want code like this to occur:

ctx.FillStyle = "rgb(255, 0, 0)"
ctx.FillStyleGradient = gradient
fmt.Println(ctx.FillStyle) // why is this magically not "rgb(255, 0, 0)" anymore? Nothing set that field!

Option 1 is also not a valid choice. If the user has to use the low level js.Object API, that's identical to our package lacking a feature.

I offer another option for consideration: discriminated unions by using an interface. Something like this:

type FillStyle interface {
  isFillStyle()
}

type StringFillStyle string
func (StringFillStyle) isFillStyle() {}

// do the same for gradient and pattern, which are conveniently already declared types

func (ctx) SetFillStyle(fs FillStyle) {
 // set the fill style
}