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

Support compiling basic Vecty/Vugu examples to WASM #93

Closed
johanbrandhorst opened this issue Nov 28, 2018 · 60 comments
Closed

Support compiling basic Vecty/Vugu examples to WASM #93

johanbrandhorst opened this issue Nov 28, 2018 · 60 comments
Labels
enhancement New feature or request wasm WebAssembly

Comments

@johanbrandhorst
Copy link
Member

johanbrandhorst commented Nov 28, 2018

This is just a tracking issue for any outstanding work required for us to compile a vecty-style web app into WASM with tinygo. I think this would be a fantastic milestone for this repo and the Go WASM community at large, so lets see what remains outstanding.

  • Channels
  • syscall/js callback support
  • a subset of reflect including
    • reflect.ValueOf, reflect.TypeOf, reflect.New and the following functions of the reflect.Value type:
      • Kind, Elem, String, Interface, Type, NumField, Field, Set

Would love to have @slimsag take a quick look at this if possible, but otherwise I think @bketelsen will know the best what is outstanding.

@aykevl
Copy link
Member

aykevl commented Nov 28, 2018

Channels (not sure this is strictly required)

Should be doable. Perhaps not select or multiple goroutines sending/receiving on the same channel, but basic send/recv between two goroutines might not be a lot of work.

  • reflect (also not sure if it's required, but I assume it is)

That's a really big one. If it is required, please tell me which part is because reflect will probably be implemented in parts.

Oh and I seem to remember vecty also needs callback support in syscall/js? That's not yet implemented AFAIK.

@johanbrandhorst
Copy link
Member Author

Updated topic with callback support, thanks!

@bketelsen
Copy link

i'm not going to have time to look or work on this for about 2 weeks, but I'd like to consider removing the reflect requirements from vecty. That might mean it's less vecty and more something new.

@johanbrandhorst
Copy link
Member Author

Interesting, would definitely be worthwhile thinking about coming at this problem from both ends, designing a Go web framework with TinyGo compatibility from the start could get us where we want significantly faster. Do you have a repo for this new framework already?

@emidoots
Copy link

I'm out of town and then on vacation for a few weeks, so I likely won't be able to follow up on this until after the holidays some time.

FWIW though the list mentioned above looks correct to me. I am very interested in adding TinyGo support to Vecty, I was actually looking into how hard it would be a few weeks back but the main kicker was the lack of reflect + syscall/js callback support.

Vecty's usage of reflect is pretty minimal, but is required. It is used for a few things:

  1. Doing a shallow copy of a component type. Example: There is a Component interface whose underlying value is *MyComponent and we need to make a copy of that the struct value (not the pointer). This is so that Vecty can "save" the previous render of your component so that subsequent renders can say "what has changed since the last time I rendered?" (the vecty.RenderSkipper interface). This is done here: https://github.com/gopherjs/vecty/blob/master/dom.go#L930-L938
  2. Copying the struct fields from one component instance to another if they are tagged with vecty:"prop"; any time a parent component returns a new &MySubComponent{} instance, Vecty keeps a single persistent component instance instead of using the new pointer value you have created and returned in your render function. This is really useful for e.g. binding event listeners to a component and not having to think about "is this the old component instance, or the new one?"; we do this here: https://github.com/gopherjs/vecty/blob/master/dom.go#L941-L967
  3. Component type comparison: We have Component(&Foo{}) and Component(&Baz{}) and we don't know if they are the same type or not (Vecty is only aware of the Component interface, not the underlying concrete types); we do that here: https://github.com/gopherjs/vecty/blob/master/dom.go#L908-L912

All other uses of reflect are just for improved debugging around printing type names, but nothing critical.

@johanbrandhorst
Copy link
Member Author

Thank you so much for chiming in @slimsag! There's no reason we can't try to achieve this with Vecty or whatever vecty-like thing we might be able to create. The requirements for vecty then are narrowed down to:

  • Channels
  • syscall/js callback support
  • a subset of reflect including
    • reflect.ValueOf, reflect.TypeOf, reflect.New and the following functions of the reflect.Value type:
      • Kind, Elem, String, Interface, Type, NumField, Field, Set

This is obviously something TinyGo wants to support eventually anyway, but the real goal of this issue is for a vecty-like framework to be possible to use with TinyGo, so it's still valuable for us consider a new framework written from scratch to avoid the use of reflect specifically, since I don't think we'll ever be able to remove the other two requirements.

@emidoots
Copy link

For sure. One thing I would mention is that Vecty certainly did not use reflection because it was convenient, but rather because the API would be abysmal otherwise. I'd be super happy to see a React-like framework that doesn't require reflection and could be fully featured.. but I just don't see it being possible without the API being really unpleasant. If anyone can prove me wrong Vecty will change 😃

@bketelsen
Copy link

Hearing that is a strong discouragement from even trying... I know far less about it than @slimsag, not much chance I can do better.

@emidoots
Copy link

Thought: One viable option to bypass the reflection requirement would be code generation. Every component would have auto-generated ShallowCopy, CopyProps, IsSameType(other Component) methods. This would kind of suck because you would need to go generate after adding any new component to your codebase, but it would bypass the reflection requirement and could be a special case behind a tinygo build tag.

Does tinygo have its own build tag?

@johanbrandhorst
Copy link
Member Author

I think code generation is definitely a viable alternative. As long as we can make that easy enough to use. I suppose we would have a generator that checks for some comment or embedded interface or something like that and generate into the same directory? If it's as easy as running vectygen ./... it should be fine I think?

@emidoots
Copy link

Yeah, it would be something like that; it would find all components by looking for structs that embed vecty.Core, and then write mycomponent.gen.go with implementations for the methods I described in my last post. That would remove the reflection requirement as long as there is a tinygo build tag or something.

Ideally that would be an intermediate solution and the long term solution here would still be the reflection approach so that code generation is not needed, since code generation is not great.

@aykevl
Copy link
Member

aykevl commented Dec 10, 2018

How much of these features (like reflect) would be necessary for a "proof of concept" hello world example? It would be nice to have some sort of absolute minimum to iterate on.

EDIT: oh and yes there is a tinygo build tag you can use.

@emidoots
Copy link

@aykevl Conclusively I think:

  • Doing a shallow copy of a component type. -> probably required (or could be hacked around by calling out to JS perhaps)
  • Copying the struct fields from one component instance to another if they are tagged with vecty:"prop" -> not required for a hello world, but would be for anything greater than that
  • Component type comparison: We have Component(&Foo{}) and Component(&Baz{}) and we don't know if they are the same type or not -> probably required but I am not positive as of time of writing this.

I am on holiday starting next week, I may try to hack something together here. I'll let you know if I do.

@aykevl
Copy link
Member

aykevl commented Dec 12, 2018

I've started work on reflection support in #104. See testdata/reflect.go for what is supported.

@deadprogram deadprogram added the enhancement New feature or request label Jan 11, 2019
@bradleypeabody
Copy link
Contributor

Fwiw, Vugu has similar requirements. vugu/vugu#39 .

I'm not sure if there are other places where reflect is used but certainly the ability to obtain reflect.Value and check the Kind, and the ability to iterate over struct fields - Vugu needs those in order to function properly. And otherwise it's maps, and syscall/js.

As mentioned in Slack, once I have some more of the core functionality of Vugu built out I'll see if I can help contribute on some of these points.

Is there any sense in creating another issue to track Vugu-related support items for TinyGo? Or is it better to just track it all here in this issue?

@aykevl
Copy link
Member

aykevl commented Apr 21, 2019

I don't think it's useful to add another issue. However, sharing what you need exactly here is very useful, like you just did.

ability to obtain reflect.Value and check the Kind

This has been implemented

iterate over struct fields

This should still be implemented. Should not be a massive amount of work, but not trivial either. (Something about priorities and hours in a day).

maps

There is very limited support, it's barely usable in its current state. Any help here is appreciated, you can take a look at the map implementation in the runtime of the main Go compiler for ideas. Wikipedia also gives some background information.

syscall/js

Many functions are implemented, but to be broadly useful more functions need to be implemented. In particular, callback support (js.FuncOf). The blocker for callback support is better support for maps, as syscall/js uses maps in its implementation.
What do you specifically need from syscall/js? Which functions/methods are absolutely necessary?

@bradleypeabody
Copy link
Contributor

Thanks for the feedback Ayke, that makes sense.

On syscall/js, it's essentially:

  • Global()
  • Value.Call()
  • The type conversion operations like Value.String() and Value.Int()
  • Value.Set() and Value.Get()
  • Value.Truthy()
  • FuncOf() (so we can register callbacks for DOM events - click handlers and the like)

While complex programs built with Vugu might have more requirements, the above should make a basic Vugu program compile and run.

Regarding map support, sounds good. I will try following the instructions here to see if I can build from source on my machine (Mac laptop) and go from there.

@johanbrandhorst johanbrandhorst changed the title Support compiling basic Vecty examples to WASM Support compiling basic Vecty/Vugu examples to WASM Apr 22, 2019
@aykevl
Copy link
Member

aykevl commented Jun 3, 2019

Update: the latest release (v0.6.0) has added support for js.FuncOf and has improved support for maps.

@emidoots
Copy link

emidoots commented Jul 1, 2019

hexops/vecty#232 has just been merged which implements WebAssembly support in Vecty using the standard Go compiler.

While this doesn't automatically give us TinyGo support (unfortunately), it does give a clear picture of how far away we are. To get compilation passing we currently need:

  • reflect.New
  • reflect.ValueOf(someStructPtrInterface).Elem().Type().Field(i).Tag.Get("vecty")

I'll see if I can create a branch of Vecty soon that at least compiles under TinyGo to see if there are any other issues.

@aykevl
Copy link
Member

aykevl commented Jul 1, 2019

Great! Now we have something to work towards.

emidoots added a commit to hexops/vecty that referenced this issue Jul 1, 2019
In my rushed attempt to merge WebAssembly support and get an announcement out at 2am last night, my sleep-deprived brain incorrectly parsed the output of `ls -s` in bytes and not kilobytes as is correctly stated in `man ls`. 🤦‍♂ 🤦‍♀

I do believe we can get much smaller bundle sizes, particularly [once we have TInyGo support](tinygo-org/tinygo#93) but it definitely wasn't my goal to mislead everyone here & in the announcement posts.

Apologies for the confusion and to everyone I have misled with this mistake!
emidoots added a commit to hexops/vecty that referenced this issue Jul 1, 2019
In my rushed attempt to merge WebAssembly support and get an announcement out at 2am last night, my sleep-deprived brain incorrectly parsed the output of `ls -s` in bytes and not kilobytes as is correctly stated in `man ls`. 🤦‍♂ 🤦‍♀

I do believe we can get much smaller bundle sizes, particularly [once we have TInyGo support](tinygo-org/tinygo#93) but it definitely wasn't my goal to mislead everyone here & in the announcement posts.

Apologies for the confusion and to everyone I have misled with this mistake!
emidoots added a commit to hexops/vecty that referenced this issue Jul 1, 2019
…242)

In my rushed attempt to merge WebAssembly support and get an announcement out at 2am last night, my sleep-deprived brain incorrectly parsed the output of `ls -s` in bytes and not kilobytes as is correctly stated in `man ls`. 🤦‍♂ 🤦‍♀

I do believe we can get much smaller bundle sizes, particularly [once we have TInyGo support](tinygo-org/tinygo#93) but it definitely wasn't my goal to mislead everyone here & in the announcement posts.

Apologies for the confusion and to everyone I have misled with this mistake!
@emidoots
Copy link

emidoots commented Jul 6, 2019

I've created a WIP / hacked together branch of Vecty which passes type checking under TinyGo: hexops/vecty#243

Unfortunately, it doesn't pass compilation yet due to this issue: #440

@bradleypeabody
Copy link
Contributor

Hi @ethanfrey I've done a fair amount of work getting Vugu to compile under Tinygo - the latest on master does now compile but functionally it's broken because of some things that need alternate implementation. Json is one of those things. I actually created the vjson package for this purpose but haven't written the Tinygo part of it yet (see https://github.com/vugu/vjson/blob/master/vjson-tinygo.go). The json parsing itself should be trivial to get working in tinygo and I believe a number of other json parsers out there will work fine - it's the more advanced reflection stuff that Tinygo has trouble with. Vugu does need some basic json support in order to work but it's minimal (I think only map[string]interface{} and basic types) and I plan on adding that into vjson as soon as possible. Otherwise most of the data transfer that Vugu does internally is encoded manually; I ended up making a simple "instruction" format for this - but that's really a Vugu-specific thing. If you are trying to transfer data from your app to a server, JSON is a good choice. And probably a third party library will do the trick.

@aykevl
Copy link
Member

aykevl commented Jan 20, 2020

Take a look at #856, which adds support for interfaces as map keys.

@ethanfrey
Copy link

Hi @bradleypeabody thank you for the answer. I actually think map[sting]interface{} would be harder without reflection than an actual struct with fixed types (as many types will only be known runtime with the map). I did look around for other json libs and found easyjson, which I will try out. I know this is not directly related to vugu, but I think we want some similar functionality. I will let you know if I get this to work.

@bradleypeabody
Copy link
Contributor

@ethanfrey Thanks and sounds good - yes, please do let me know how that goes.

To shed a little more light on it - the thing about map[string]interface{} where each of the values is one of the primitive types makes it possible to implement without using reflect.New(), which would otherwise be required to be able to instanciate a struct.

@marwan-at-work
Copy link

any updates here? :)

@bradleypeabody
Copy link
Contributor

Although not entirely a blocker, module support in Tinygo would be a big help for Vugu. Vugu supports re-building your application on the fly (upon page refresh and I also just added a websocket auto reloader so editing a vugu file refreshes the page automatically). These and other tools and features are only supported and tested in conjunction with modules.

My current plan is that when Tinygo has module support I will dive back in and try to get Vugu+Tinygo working. I saw this PR #941 but am not sure how close this is to being functional.

(Although even as I write this I'm coming up with ideas for workarounds, but yeah, modules would make things much simpler.)

@deadprogram
Copy link
Member

Hi @bradleypeabody thank you for the feedback. Module support is an important feature, and we will likely return to it after the release that we are currently preparing for this week, which adds Go 1.14 and LLVM 10 support.

If you have a chance to try out the latest dev branch with some of your code, it would be greatly appreciated to see if any of your previous issues are now resolved.

Thanks!

@bradleypeabody
Copy link
Contributor

@deadprogram understood and thanks for the info. There are some challenges involved on my side but this is definitely on my todo list and I'll get back to you as soon as I can. (I do have a workaround in mind to make things work without modules and I'll try that out and see how far I can get.)

@bradleypeabody
Copy link
Contributor

FYI good progress is being made on this. With this issue #1091 now resolved enough to move forward, for Vugu the ball is back in my court to start testing and trying out more complex examples. But basic page rendering and event handling plus re-render loop (i.e. stuff shows up and you can click buttons that toggle things on and off) is working now and with executables that are around 250k.

@johanbrandhorst
Copy link
Member Author

That is fantastic news Brad, congratulations to everyone involved!

@deadprogram deadprogram added the wasm WebAssembly label May 13, 2020
@bradleypeabody
Copy link
Contributor

Vugu+TinyGo support now has basic functionality along with an example and some documentation: https://www.vugu.org/doc/tinygo . A majority of the Vugu test suite also now runs both the default Go compiler and TinyGo. There are still some test cases that don't work, but most of them do and I think it's working well enough to start trying it out.

I'll update with anymore specific issues that arise. I know http client support (i.e. making net/http compile and having http.Get map to JS fetch()) is one that will come up soon. I plan on doing some experimentation soon to see how much effort that would take to resolve.

@johanbrandhorst
Copy link
Member Author

Awesome news Brad, thank you so much for your contributions to this project!

@aykevl
Copy link
Member

aykevl commented Jun 23, 2020

That sounds great!

I saw that https://www.vugu.org/doc/tinygo says that modules are not supported:

By default Docker is used to call TinyGo. You can use wc.NoDocker() to disable this and have Vugu call the local tinygo executable from your path. Note that for now this results in various additional temporary folder structures being created during compilation to get the GOPATH setup correctly and thus is usually slower. Once TinyGo supports Go Modules, this limitation should go away.

By now, TinyGo does in fact support modules: #941.

@bradleypeabody
Copy link
Contributor

bradleypeabody commented Jun 23, 2020

@aykevl Nice! I didn't realize that. I've updated the doc page text for accuracy

... Vugu should be updated soon to remove this limitation and utilize TinyGo's module support.

And I will try this out. If everything plays well together I can simplify a few things and I'll see if it makes sense to make the local (non-Docker) call approach be the default. I suspect as TinyGo gets better and better the advantages of the Docker approach will become fewer and fewer (and not be worth the added overhead).

@ethanfrey
Copy link

@bradleypeabody I took a look at Vugu and am quite impressed with how much you managed to get into Wasm, and compile with TinyGo. My respect.

I am the CTO of https://github.com/CosmWasm/cosmwasm , a blockchain smart contracting module (that can plug into many different blockchains). We run Wasm modules and currently only have a Rust compile path. After watching TinyGo (and your progress with it) for some time, we have decided to start trying to port our smart contract framework to Go, and support both languages (Most blockchains we integrate with are written in Go).

I started looking at your code, and would love to reuse some of it (like https://github.com/vugu/vjson) and ideally contribute back in some way if I can. Is there a better way to reach out to you than responding to this issue?

@bradleypeabody
Copy link
Contributor

@ethanfrey Will reply separately.

@marwan-at-work
Copy link

Hi there 👋

Any updates on TinyGo working with Vecty? Thank you!

@deadprogram
Copy link
Member

This was released with v0.14.0 so now closing. Please reopen if needed. Thanks!

@deadprogram deadprogram removed the next-release Will be part of next release label Aug 7, 2020
@marwan-at-work
Copy link

@deadprogram thank you for the update!

Trying to compile my program with tinygo results in the following errors from encoding/json

tinygo build -o wasm.wasm -target=wasm main.go
# encoding/json
/usr/local/go/src/encoding/json/decode.go:160:15: cannot convert nil (untyped nil value) to reflect.Type
/usr/local/go/src/encoding/json/decode.go:235:26: cannot convert nil (untyped nil value) to reflect.Type
/usr/local/go/src/encoding/json/decode.go:252:30: cannot convert nil (untyped nil value) to reflect.Type
/usr/local/go/src/encoding/json/decode.go:532:8: v.NumMethod undefined (type reflect.Value has no field or method NumMethod)
/usr/local/go/src/encoding/json/decode.go:569:7: v.SetLen undefined (type reflect.Value has no field or method SetLen)
/usr/local/go/src/encoding/json/decode.go:606:6: v.SetLen undefined (type reflect.Value has no field or method SetLen)
/usr/local/go/src/encoding/json/decode.go:637:40: v.NumMethod undefined (type reflect.Value has no field or method NumMethod)
/usr/local/go/src/encoding/json/decode.go:658:16: PtrTo not declared by package reflect
/usr/local/go/src/encoding/json/decode.go:793:17: PtrTo not declared by package reflect
/usr/local/go/src/encoding/json/decode.go:938:9: v.NumMethod undefined (type reflect.Value has no field or method NumMethod)
/usr/local/go/src/encoding/json/decode.go:967:6: v.SetBytes undefined (type reflect.Value has no field or method SetBytes)
/usr/local/go/src/encoding/json/decode.go:974:9: v.NumMethod undefined (type reflect.Value has no field or method NumMethod)
/usr/local/go/src/encoding/json/decode.go:1007:9: v.NumMethod undefined (type reflect.Value has no field or method NumMethod)
/usr/local/go/src/encoding/json/decode.go:1031:23: v.OverflowFloat undefined (type reflect.Value has no field or method OverflowFloat)
/usr/local/go/src/encoding/json/encode.go:420:53: PtrTo not declared by package reflect
/usr/local/go/src/encoding/json/encode.go:426:53: PtrTo not declared by package reflect
/usr/local/go/src/encoding/json/encode.go:865:16: PtrTo not declared by package reflect

Here's a simple program to reproduce it:

package main

import "encoding/json"

type t struct {
	One string `json:"one"`
}

func main() {
	var t t
	json.Unmarshal([]byte(`{"one": "two"}`), &t)
}

@deadprogram I'm not sure if this is a known issue and if it's unrelated to compiling tiny go with vecty but thought I'd bring it up here since frontend applications will most likely use the json library for api calls.

Thanks!

@ethanfrey
Copy link

@marwan-at-work encoding/json does not work with TinyGo as it makes extensive use of the reflect package (which is not fully supported by TinyGo for various technical reasons).

@bradleypeabody made an amazing package https://github.com/vugu/vjson which provides TinyGo-compliant JSON parsing and unmarshalling.

@marwan-at-work
Copy link

marwan-at-work commented Aug 7, 2020

@ethanfrey thank you! I will definitely check that library out.

Encoding/json aside, I tried compiling a basic Vecty app (without importing json) and got the following error:

root@05844ebaa1e7:/app# tinygo build -o wasm.wasm -target=wasm .
# github.com/gopherjs/vecty
/go/pkg/mod/github.com/gopherjs/[email protected]/dom_wasmjs_gopherjs.go: interp: unknown GEP

traceback:
github.com/gopherjs/vecty/<init>:41:24:
  %15 = call %runtime._interface @"github.com/gopherjs/vecty.wrapObject"([0 x %runtime.funcValue] %12, i64 %13, i64* %14, i8* undef, i8* undef), !dbg !216

Using this simple program:

package main

import (
	"github.com/gopherjs/vecty"
	"github.com/gopherjs/vecty/elem"
)

type comp struct {
	vecty.Core
}

func (c *comp) Render() vecty.ComponentOrHTML {
	return elem.Div(vecty.Text("hello"))
}

func main() {
	vecty.RenderBody(&comp{})
}

Again, not sure if it's a vecty specific issue or a tiny-go issue with reflect but thought I'd post it here and happy to report it anywhere else that needs to be.

Thanks again for the quick response and all the work on tiny go!

@marwan-at-work
Copy link

Should we re-open the issue in this case? Or make a new one to track the progress on whatever is needed to compile vecty programs? Thanks

@deadprogram
Copy link
Member

I would suggest check out https://www.vugu.org/doc/tinygo for specifics.

@marwan-at-work
Copy link

@deadprogram FWIW, I was mainly talking about vecty not vugu, and the page you linked me is very vugu specific and doesn't offer any hints on how to make vecty work :)

The above program compiles with Go but it does not compile with TinyGo which makes me think it's probably more appropriate to file as a TinyGo bug rather than a Vecty bug.

I definitely understand that TinyGo still doesn't fully support reflection, but I was under the impression that this issue was to track its progress since it was what was missing for Vecty to compile with tiny go.

If that's the case, I'd be happy to re-open the issue, or maybe open a separate tracking issue.

Otherwise, I'm happy to go with the follow on whichever is the best way to solve this issue.

Thanks again and appreciate all of the work and help here!

@johanbrandhorst
Copy link
Member Author

I think closing this issue is appropriate as it is now possible to use Vugu with TinyGo (:tada:). Vugu and TinyGo had to meet a bit in the middle on this, and I think it might be reasonable to expect the same thing from Vecty. Therefore, I think we should open a separate issue to track the outstanding work, if any, for supporting Vecty. We should port over the current errors (thanks @marwan-at-work for trying it!) so we know where the shortcomings are, and if we should tackle them in TinyGo or Vecty. I'd say the above error is a TinyGo one, but it's not unreasonable for Vecty to provide a less reflect-heavy implementation if it helps bridge the gap, like Vugu did.

How does that sound?

@marwan-at-work
Copy link

marwan-at-work commented Aug 7, 2020

@johanbrandhorst done :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wasm WebAssembly
Projects
None yet
Development

No branches or pull requests

9 participants