RenderBody became blocking (it never returns) after PR #232

dmitshur commented 5 years ago

In one of my applications, I've been using vecty's API roughly as follows:

vecty.RenderBody(body)
for {
    // sleep or wait for event ...
    vecty.Rerender(body)
}

It's possible I'm not using vecty API correctly, but that's what I came up with based on reading the documentation and experimenting (a while ago). It worked without noticeable issues until PR #232.

According to https://github.com/gopherjs/vecty/blob/master/doc/CHANGELOG.md#june-30-2019-pr-232-major-breaking-change, there have not been changes to vecty.RenderBody behavior.

The documentation of vecty.RenderBody says:

RenderBody renders the given component as the document body. The given Component's Render method must return a "body" element.

I don't see anything that suggests that vecty.RenderBody would block execution and never return.

However, as of PR #232, that's what it does:

https://github.com/gopherjs/vecty/blob/2b6fc20f8913c7dadc6a67786be15c8c92bbd16a/dom.go#L1194-L1195

Which means my application never proceeds past the vecty.RenderBody(body) call.

Am I doing something wrong, or is this a bug/regression?

nobonobo commented 5 years ago

Maybe, due to support for WASM. If returned function of "main", WASM instances will all dispose.

dmitshur commented 5 years ago

I understand it's likely related to WebAssembly, but it doesn't tell me if this was intentional or not. The user program can arrange for the main function not to return, vecty doesn't necessarily have to take on that responsibility.

If it's meant to be intentional, I believe it should be documented. The user shouldn't have to look into the source code to know a function will never return.

Further, how is a vecty app that needs to re-render the main body after the initial render expected to do so? I looked at the examples and the most informative one seems to be todomvc:

https://github.com/gopherjs/vecty/blob/2b6fc20f8913c7dadc6a67786be15c8c92bbd16a/example/todomvc/example.go#L22-L26

It sets up some listeners in advance of calling vecty.RenderBody. Is that the only supported approach, or are other approaches okay too? Thanks!

/cc @slimsag

slimsag commented 5 years ago

Apologies for the delayed response here!

Yes, this change was intentional -- but I clearly should've thought out the consequences more than I did.

Why did we change anything?

When investigating WebAssembly support, it became clear we needed to do something here because of the fact that WebAssembly and GopherJS differ in how they handle goroutines that are executing after the main goroutine returns.

In GopherJS, background goroutines (such as ones that would run on a click event, for example) continue execution even after main returns. That is, in GopherJS once RenderBody returns other goroutines in the application still continue to execute.

By contrast, in WebAssembly with the Go compiler, once main returns all other goroutine execution is halted. This gave us two choices (which you noted above):

Make the user application responsible for keeping other goroutines alive. In practice, this would be the following change to hellovecty and other applications:

...

func main() {
    vecty.SetTitle("Hello Vecty!")
    vecty.RenderBody(&PageView{})
    select{} // keep Go alive (you can just ignore this for now, but it is needed)
}

...

Handle it as part of vecty.RenderBody which is the "entrypoint" of sorts to any application using Vecty. This was the option I went with.

Forethought / decision making

The main reason for going with #2 above instead of #1 was that #1 introduces an additional complexity that beginners would need to understand and reason about. In my experience, when learning something, seeing somewhat magical lines of code that are dire to an application working is not super friendly.

Initially, I had just planned to use select{} when compiling under WebAssembly only in order to preserve complete compatibility for GopherJS applications. After thinking about this further, I came to the conclusion that it would be weird if the behavior changed under the compilation target and I would like to hide that complexity from users instead.

This is, clearly, when I dropped the ball a bit. Thinking of existing Vecty applications, I couldn't think of any case where someone would not be calling vecty.RenderBody last because (in my mind) I thought anyone doing so would be relying on some very weird behavior which also wouldn't be compatible in the WebAssembly world. This was wrong.

I also incorrectly thought that this part of the description would communicate no guarantee about how quickly rendering occurs:

RenderBody renders the given component as the document body. [...]

Should async usages be supported?

[...] It sets up some listeners in advance of calling vecty.RenderBody. Is that the only supported approach, or are other approaches okay too?

This is a good, but also difficult, question.

One possibility I had considered was that if there was a compelling reason to keep this functionality (users owning the 'main loop'), we could retain it via a new method like RenderBodyAsync or something similar.

The main reason I thought of when thinking about why we would support this was to allow for Go WebAssembly applications which already do some work on their own and own a main goroutine (e.g. a WebGL video game written in Go) to make use of Vecty. I remain convinced this is a compelling reason to add this.

What I hadn't considered was that this could also be useful in applications such as yours where there is an existing scheduler and Vecty is being integrated more ad-hoc. In other words, I don't think of this as the canonical / most correct way to write a Vecty app but at the same time I don't have enough knowledge of the G-P-S codebase in order to say for certain which changes I would make. I'll try to find some time to investigate from this angle.

Next steps

I completely agree this should be documented, both in the function docstring and in the breaking changes section of the changelog in light of this.
I will add RenderBodyAsync (or perhaps just RenderInto i.e. https://github.com/gopherjs/vecty/issues/81 which would be async) which would support this scenario.

I'll do my best to land both of the above this weekend

Apologies for the trouble this caused and for my not considering this case! I want Vecty to be the go-to web frontend for Go, and clearly need to do better here in order to get there! I consider G-P-S to be a shining star of sorts as far as Vecty real-world use cases go, so I also want to support you better there :)

dmitshur commented 5 years ago

Thank you very much for such a thoughtful and detailed answer @slimsag!

I don't mind making changes in my codebase to update to a new/different API, but I couldn't do that without first understanding whether this was an intentional change or not. Your reply has helped clarify that.

I want to confirm my understanding of another question. I was considering using the current API like this:

go func() {
    for {
        // sleep or wait for event ...
        vecty.Rerender(body)
    }
}()
vecty.RenderBody(body)

But my current understanding is that it's not safe to do that, because there's a race condition. If the event arrives too quickly, vecty.Rerender(body) can be called before vecty.RenderBody(body) has completed running. Do you concur?

slimsag commented 5 years ago

I'm not near a computer at the moment, so I cant reply in full, but thats correct and Rerender would panic in particular because we cannot rerender components that haven't been rendered once before

On Fri, Aug 16, 2019, 6:59 PM Dmitri Shuralyov notifications@github.com wrote:

Thank you very much for such a thoughtful and detailed answer @slimsag https://github.com/slimsag!

I don't mind making changes in my codebase to update to a new/different API, but I couldn't do that without first understanding whether this was an intentional change or not. Your reply has helped clarify that.

I want to confirm my understanding of another question. If I were to consider using the current API like this:

go func() { for { // sleep or wait for event ... vecty.Rerender(body) } }() vecty.RenderBody(body)

But my current understanding is that it's not safe to do that, because there's a race condition. If the event arrives too quickly, vecty.Rerender(body) can be called before vecty.RenderBody(body) has completed running. Is that right?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gopherjs/vecty/issues/247?email_source=notifications&email_token=AAYGWODXUUO3C4CRNX2WAF3QE5LRXA5CNFSM4IKYQ3LKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4QBBJI#issuecomment-522195109, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYGWOHTQ6KP2UIVJ2OE5ITQE5LRXANCNFSM4IKYQ3LA .

dmitshur commented 4 years ago

Friendly ping on this issue to indicate I'm still interested in this (with full understanding this is open source, so there are no expectations of work).

This is blocking me from making updates to Go Package Store, so I may end up making a temporary fork to work around this. (It's not a problem if I have to do that; thanks for all your work so far that I could benefit from.)

slimsag commented 4 years ago

Thanks for the ping and apologies for the massive delay! I've just merged the fix now: #249

To fix G-P-S, you can simply replace all current blocking calls of vecty.RenderBody(comp) with the non-blocking form:

if err := vecty.RenderInto("body", comp); err != nil {
    panic(err)
}

Documentation is of course available on godoc:

If this works, or doesn't and gives you trouble, please let me know! :)

dmitshur commented 4 years ago

Thank you very much Stephen!

hexops / vecty