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

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

Open
dmitshur opened this issue Aug 10, 2019 · 5 comments

Comments

@dmitshur
Copy link
Member

commented Aug 10, 2019

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:

vecty/dom.go

Lines 1194 to 1195 in 2b6fc20

// run Go forever
select {}

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

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

@nobonobo

This comment has been minimized.

Copy link

commented Aug 12, 2019

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

@dmitshur

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

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:

store.Listeners.Add(p, func() {
p.Items = store.Items
vecty.Rerender(p)
})
vecty.RenderBody(p)

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

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

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):

  1. 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)
}

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

  1. 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.
  2. I will add RenderBodyAsync (or perhaps just RenderInto i.e. #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

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.