go-app icon indicating copy to clipboard operation
go-app copied to clipboard

app.If() is taken with care as it always evaluates the element list.

Open oderwat opened this issue 4 years ago • 6 comments

It is somewhat obvious but can lead to strange bugs anyway. If one uses app.If(), it always evaluates the element list. This may decrease performance and even lead to crashes. So it may be better if we have an implementation that takes functions instead.

oderwat avatar Oct 31 '21 22:10 oderwat

I feel If is already heavy to use in terms of syntax. Changing the API to take a func would worse the thing.

maxence-charriere avatar Nov 01 '21 03:11 maxence-charriere

Well, I have such implementation for evaluation in my version and it works well. I was thinking about it being too cumbersome but having the render routine in one long streak makes it easier to understand what happens. I am undecided and this is also why I didn't make a PR for it. I almost think the current offerings of If() / elseIf() / else() is an error because the evaluations are always overhead.

oderwat avatar Nov 01 '21 11:11 oderwat

I think that using something like this makes more sense than the app.If() construct and that app.If() should be deprecated and give such an example ;-)

func (c *thing) Render() app.UI {
	return func() app.UI {
		if something {
			return app.Div()."something is true")
		} else {
			return nil
		}
	}()
}    

Or as part of a real use case which also shows why app.If() is not sufficient. This renders the tabs and then the content for the currently selected tab. It can do this either by rendering all tabs and show hide some or use my mountpoint package to actually switch the component (even if they are the same type)

... Render() app.UI {
... return Div().Body(
	// Tab states are rendered
	),
	// Tab content is rendered
	func() app.UI {
		if c.mountPoint != nil {
			return app.Div().Class("pt-5").Body(c.mountPoint.UI())
		} else {
			return app.Range(c.Tabs).Slice(func(i int) app.UI {
				var state string
				if i == c.Active {
					state = "block"
				} else {
					state = "hidden"
				}
				return app.Div().Class("pt-5", state).Body(c.Tabs[i].Compo)
			})
		}
	}(),

oderwat avatar Nov 01 '21 14:11 oderwat

I agree with this, but I don't know if it's fixable. It has lead to me avoid using app.lf, or at least being very mindful when I do.

ZackaryWelch avatar Nov 20 '21 01:11 ZackaryWelch

fwiw I have already hit crashes 2 times in my first app, by using app.If() and not remember that its always evaluated, and that code path has a null pointer when its not the active branch. I've switched to doing my own branch ahead of time. It would be better to deprecate app.If and provide an alternate app.IfFunc to take an anonymous function and defer the evaluation until its the active branch.

justinfx avatar Aug 18 '22 02:08 justinfx

Let's just start with changing the Hello World example from README.md:

https://github.com/maxence-charriere/go-app#declarative-syntax

Here is a Hello World component that takes an input and displays its value in its title

Else, this will be the first thing people would try and then form the habit of using it, thinking this is the recommended approach.

suntong avatar Aug 14 '23 17:08 suntong