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

Add a Iff helper #172

Merged
merged 4 commits into from
Jun 19, 2024
Merged

Add a Iff helper #172

merged 4 commits into from
Jun 19, 2024

Conversation

JulienTant
Copy link
Contributor

@JulienTant JulienTant commented Jun 5, 2024

I ran into some situation where I want to conditionally render a node if some variable is not nil and obviously I got a panic

// package viewmodels
type SomePage struct {
	s *string
}

// package views
func SomePage (vm viewmodels.SomePage) g,Node {
	return Div(
		If(vm.s == nil, Text("s is nil"),
		If(vm.s !- nil, Text("s is " + vm.s), // this will panic when `s` is nil
	)
}

In this situation, go will interpret the code of the second if regardless of the condition because the code itself is not in a condition.

This PR introduces a new Iff helper that accepts a callback. The callback content is only interpreted when it's called, making the code safe:

// package viewmodels
type SomePage struct {
	s *string
}

// package views
func SomePage (vm viewmodels.SomePage) g,Node {
	return Div(
		Iff(vm.s == nil, func () g.Node { return Text("s is nil") },
		Iff(vm.s !- nil, func () g.Node { return Text("s is " + vm.s) },
	)
}

I'm aware of the Lazy effort on the side, but I guess this is no a breaking change and can still exist in addition to the Lazy effort.

@markuswustenberg
Copy link
Member

Hi @JulienTant , thank you for the PR! I've done something very similar in #99, and actually attempted to resolve this in #101 and #156 as well. I've never reached a solution I really like, essentially because I think it's confusing to have two different conditional functions from a user perspective.

I really would have liked the foresight to just implement If this way, because you're definitely not the first to hit this… I don't know what to call it, "curiosity". But maybe this very straightforward solution is just the simplest way.

Between your solution here and the Lazy approach, which would you prefer?

@JulienTant
Copy link
Contributor Author

JulienTant commented Jun 6, 2024

I think I like having two separate options because not all the Ifs need a callback, amd callback just ain't pretty in go. It's not like we can have

If(things == nil, () => P(g.Text("No things"))

Looking at the stdlib's slices for example, most functions also have a *Func equivalent so this is not shocking.

As an alternative, we could promote a new callback type and it works natively with If:

type NodeCallback func() g.Node

func (f NodeCallback) Render(w io.Writer) error {
    return f().Render(w)
}

func TestIfWithNodeCallback(t *testing.T) {
    t.Run("return the nil case without panicking", func(t *testing.T) {
        var nilable *string
        n := g.El("div",
            g.If(nilable != nil, NodeCallback(func() g.Node {
                return g.El("span", g.Text(*nilable))
            })),
            g.If(nilable == nil, NodeCallback(func() g.Node {
                return g.El("span", g.Text("No messages."))
            })),
        )
        assert.Equal(t, "<div><span>No messages.</span></div>", n)
    })

    t.Run("returns the non nil case", func(t *testing.T) {
        var notNil *string = new(string)
        *notNil = "You lost your hat!"
        n := g.El("div",
            g.If(notNil != nil, NodeCallback(func() g.Node {
                return g.El("span", g.Text(*notNil))
            })),
            g.If(notNil == nil, NodeCallback(func() g.Node {
                return g.El("span", g.Text("No messages."))
            })),
        )
        assert.Equal(t, "<div><span>You lost your hat!</span></div>", n)
    })
}

@markuswustenberg
Copy link
Member

@JulienTant I've been thinking about this.

Your NodeCallback is essentially the Lazy type I've been implementing in #156, and although it's fine, it's a bit awkward to make it support both elements and attributes (like I did with Lazy).

What if we instead adopted a pattern of a function that is immediately evaluated? Something like this: https://go.dev/play/p/mlByJAc-Upp

package main

import (
	"os"

	g "github.com/maragudk/gomponents"
)

func main() {
	_ = g.If(true, func() g.Node {
		return g.El("div")
	}()).Render(os.Stdout)
}

I'm not sure how to make that work with the nil pointer panics without having another if statement in the function though…

Otherwise, I'm leaning towards adding your Iff. I think it's simple and elegant.

@markuswustenberg
Copy link
Member

Sidenote: I like the conciseness of Iff (and the reference to math's if-and-only-if, right?), but the consistency and obviousness of IfFunc. Which would you prefer?

@JulienTant
Copy link
Contributor Author

As you mentioned, having a self-executing function here would result in the same problem when trying to deal with nil's

var n *string
_ = g.If(n != nil, func() g.Node {
	return g.Text(n)  // will panic
}()).Render(os.Stdout)

I prefer Iff and its if and only if association, mostly because IfFunc makes me believe that the if gets the func:

func IfFunc(f func() bool, n g.Node) g.Node {
	if (f()) {
		return n
	}
	return nil
})

I think the big advantage of this solution is that it does not require any overhead for devs compared to the Lazy approach where you have to specify if it's an element or an attribute

@markuswustenberg
Copy link
Member

@JulienTant I think I'm convinced about going with Iff. Thank you so much for discussing this with me, it's been a "gotcha" pretty much since the project started gaining traction. :D

I'll merge this and update some docs and readmes, and then release it sometime soon.

@markuswustenberg markuswustenberg merged commit dafb3da into maragudk:main Jun 19, 2024
8 checks passed
This was referenced Jun 19, 2024
@JulienTant JulienTant deleted the iff branch June 20, 2024 15:03
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 this pull request may close these issues.

None yet

2 participants