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

Inline if with nil pointer #152

Closed
onordgren opened this issue Oct 25, 2023 · 8 comments
Closed

Inline if with nil pointer #152

onordgren opened this issue Oct 25, 2023 · 8 comments

Comments

@onordgren
Copy link

Hello! Ran into the following issue today while trying gomponents. When using the inline if, g.If it seems like the node is evaluated even though the statement is false. Made a small example app below, where session s is nil. There is an if statement checking if the session is not nil, and then proceeding to show the username. The code however panics with a nil reference error. Looking at the source code it would seem like the node wouldn't be evaluated, but maybe something else is going on?

package main

import (
	"net/http"

	g "github.com/maragudk/gomponents"
	h "github.com/maragudk/gomponents/html"
)

type session struct {
	Username string
}

func main() {
	s := getSession()

	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		g.Node(
			h.Div(g.Text("Hello!"),
				g.If(s != nil,
					h.Div(g.Text(s.Username),
						g.Text("You are logged in!"),
					),
				),
			),
		).Render(w)
	})

	http.ListenAndServe(":8080", nil)
}

func getSession() *session {
	return nil
}
@markuswustenberg
Copy link
Member

Hi @onordgren , thank you for taking the time to file an issue! My answer time is a bit longer than usual, I've got the flu. 🤧

I agree that this is surprising behaviour, but actually a consequence of the design of gomponents itself. As you probably know, a component is basically (most often) a function. The functions are called inline in your example, and the component given to the g.If is always evaluated, but the result may be discarded. That's the reason for your nil pointer error.

I've explored possible solutions for this in #99 and #101, but haven't found a good one yet. Maybe I should use this opportunity to revisit the problem. Feedback and input appreciated. 😊

@markuswustenberg
Copy link
Member

I tried an implementation for a solution, but I'm not entirely satisfied. If anyone wants to have a look and post their thoughts, they are very welcome!

@egeozcan
Copy link

Hi, a new user of the library here!

My humble suggestion is deprecating g.If and suggesting creating a function with an early nil return if the parameter does not satisfy. The language simply does not have the extensibility to enable any more elegant solution. But of course, I may be wrong and any pointers to alternative approaches would help a lot.

@markuswustenberg
Copy link
Member

Hi @egeozcan , thanks for chiming in.

What you're proposing is essentially what the current implementation does:

func If(condition bool, n Node) Node {
	if condition {
		return n
	}
	return nil
}

The problem lies not in what the function does, but when parameters are evaluated when using it. Because gomponents is all just nested functions, the innermost function and its parameters is evaluated before we even reach the If, and that's where the nil panics occur. The only way around that is to guard against nil in a custom inline function (which returns a Node), or check for nils earlier in the component and behave appropriately.

Or am I misunderstanding your suggestion?

@egeozcan
Copy link

hi @markuswustenberg, well I meant not using a generic function, and using a custom function to do this. the g.node is super easy to compose so I humbly suggested that, that should be encouraged instead of offering abstractions that can't cover some cases such as the one discussed and may cause confusion. thanks for the detailed reply 😊

@markuswustenberg
Copy link
Member

Hmm. Yeah, I get your point. I still think If has value, though, because it's so easy to inline components conditionally that would otherwise need to be branched elsewhere. Alternatively, one can use the IIFE pattern from Javascript-land (https://en.wikipedia.org/wiki/Immediately_invoked_function_expression).

markuswustenberg added a commit that referenced this issue Dec 12, 2023
`Lazy` provides lazy evaluation of node rendering. Useful together with `If`.

Fixes #152
@markuswustenberg
Copy link
Member

@egeozcan @onordgren if you like and have the time, I'd be happy if you had a look at my proposed lazy evaluation solution at #156. I think I've finally arrived at an API I like.

@markuswustenberg
Copy link
Member

Finally landed on a satisfying solution in #172.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants