chasefleming / elem-go

Type-safe Go library for creating and manipulating HTML elements (with htmx helpers).
MIT License
317 stars 27 forks source link

Omitting `else` clause in `If` method #67

Open whisk opened 12 months ago

whisk commented 12 months ago

Problem

There are cases when else clause in conditional rendering is not needed, but there is no way to omit the else argument. This results in somewhat awkward code:

shouldWeShowLink := true
link := elem.A(attrs.Props{attrs.Href: "/foobar"}, elem.Text("link"))

content := elem.Div(nil,
    elem.P("Some text"),
    elem.If(shouldWeShowLink, link, "")
)

Possible solutions

Add new method Introduce new method like If but with only then argument.

Pros: readable, simple Cons: It's difficult to come up with a name though.

Introduce method chaining So we could write something like:

If(cond).Then(elem).Else(elem)
If(cond).Then(elem)

Pros: readable Cons: not clear how to handle errors when then part is missing; could be a breaking change unless new package or method name is used

chasefleming commented 12 months ago

@whisk This is a great point to raise. I had actually already been thinking about multiple cases issue, but hadn't considered just the "if true do this, otherwise omit" case. I can think of a couple ways to handle it with the existing patterns (there is no other chaining in the library so I'm not sure that feels right here)...a couple options could be:

For the two or more case, I had been thinking a switch pattern might be nice. Something like:

userRole := "editor"

content := elem.Div(nil, 
    elem.Switch(userRole, 
        Case{"admin", elem.Text("Admin Dashboard")},
        Case{"editor", elem.Text("Editor Tools")},
        Case{"viewer", elem.Text("Viewer Section")},
        Default(elem.Text("Unknown Role"))
    )
)

or

userRole := "editor"

content := elem.Div(nil, 
    elem.Switch(userRole, 
        Case{"admin", elem.Text("Admin Dashboard")},
        Case{"editor", elem.Text("Editor Tools")},
        Case{"viewer", elem.Text("Viewer Section")},
        Default{elem.Text("Unknown Role")}
    )
)

Curious to hear your thoughts. What approaches do you like?

gedw99 commented 12 months ago

@chasefleming those 2 examples of switch are identical to me..

Both are fine to me. @whisk any thoughts.

My only 2 cents here are that very often these auth role / groups are data driven because they are changeable in the System at runtime. This means that your checking against a string from the db ( or whatever your using )..

chasefleming commented 12 months ago

@gedw99 they are almost identical. The only difference is in the Default case. One being a function and the other following a similar syntax as the Case. And yeah fair, maybe the wrong example. You could imagine a situation for a Switch though that is based off something like navigation where if a user selects "Home" one view is displayed or if "About" then another view is displayed. These could be changed dynamically from and htmx post. A Switch like If could also be applied for attributes and styles.

I was thinking about it more and while I like nil the most I'm not sure it will work with the types without forcing a syntax change to something like:

trueStr := "true case"

result := If(true, &trueStr, nil)

I don't love changing it to that so maybe the best option is If, IfElse, and Switch.

Although one more question for everyone: is it odd that you can use elem.If or any other conditional function like that from the elem package on things like attrs and styles? Would it be nice to have duplicate logic in the attrs and styles packages so it was more intuitive to do styles.If and attrs.If or is that more confusing? Another option would be a utils subpackage but I'd like to avoid too many of those.

Curious to hear thoughts. Thanks for the great discussion!

chasefleming commented 12 months ago

@whisk @gedw99 I just thought of one other non-breaking option: we could introduce a None function that you could use like so elem.If(true, elem.Text("hi"), elem.None()) although you'd probably need to write elem.If[elem.Node](true, elem.Text("hi"), elem.None()) with the current generics, so in this case it would probably best to break conditionals out to their own subpackages as well so you don't have to write the type.

gedw99 commented 12 months ago

I say go with whatever works now.

On we hook up a db up then we can revisit anything like this ..

whisk commented 11 months ago

Hi @chasefleming, @gedw99, thanks for sharing your ideas!

elem.None() seems to be a handy method, and can be used on other occasions when we need to create an empty element, like this:

Div(nil, None())
-> 
"<div></div>"

So I agree that it could be beneficial. Still If(shouldWeShowLink, link, None()) looks only marginally better than elem.If(shouldWeShowLink, link, Text("")) for me.

It's possible to make If using variadic args like this:

func If[T any](condition bool, branches ...T) T { ... }

If(condition, thenElem)
if(condition, thenElem, elseElem)

But there is no way for proper compile-time checks unfortunately. This is also true for struct argument patters like this:

elem.Switch(userRole, 
        Case{"admin", elem.Text("Admin Dashboard")},
        Case{"editor", elem.Text("Editor Tools")},
        Case{"viewer", elem.Text("Viewer Section")},
        Default{elem.Text("Unknown Role")}
    )

So if you forget about "then" or "default" you wouldn't know about. We can output errors directly into resulting HTML inside comment tags, but it still not quite reliable.

So what are you thoughts?

gedw99 commented 11 months ago

Hi @chasefleming, @gedw99, thanks for sharing your ideas!

elem.None() seems to be a handy method, and can be used on other occasions when we need to create an empty element, like this:

Div(nil, None())
-> 
"<div></div>"

So I agree that it could be beneficial. Still If(shouldWeShowLink, link, None()) looks only marginally better than elem.If(shouldWeShowLink, link, Text("")) for me.

It's possible to make If using variadic args like this:

func If[T any](condition bool, branches ...T) T { ... }

If(condition, thenElem)
if(condition, thenElem, elseElem)

Agree its good enough for now..

But there is no way for proper compile-time checks unfortunately. This is also true for struct argument patters like this:

elem.Switch(userRole, 
        Case{"admin", elem.Text("Admin Dashboard")},
        Case{"editor", elem.Text("Editor Tools")},
        Case{"viewer", elem.Text("Viewer Section")},
        Default{elem.Text("Unknown Role")}
    )

So if you forget about "then" or "default" you wouldn't know about. We can output errors directly into resulting HTML inside comment tags, but it still not quite reliable.

So what are you thoughts?

Strong typing loss is fine. Its reality that at some point you loose it with data binding.

In summary, I think it's decent solution.

chasefleming commented 11 months ago

Sorry for the slow response. This issue required some thought. It's a tricky one.

So if you forget about "then" or "default" you wouldn't know about. We can output errors directly into resulting HTML inside comment tags, but it still not quite reliable.

@whisk You raised an excellent point about potentially omitting 'then' or 'default' cases. And with the variadic you can't limit the dev from adding too many conditions which may be confusing. With the variadic approach, there's a risk of developers adding too many conditions, which could lead to confusion. Type safety is a key advantage of this library, and we aim to minimize runtime errors. In my opinion, the Switch structure, if we alter its syntax slightly from my earlier comment to clearly require a conditional, default, and at least one case, is less prone to runtime errors:

func Switch[T any](key string, defaultCase Default[T], cases ...Case[T]) T {
    // ...logic...
}

// Usage
result := Switch(userRole,
    Default{elem.Text("Unknown Role")},
    Case{"admin", elem.Text("Admin Dashboard")},
    Case{"editor", elem.Text("Editor Tools")},
    Case{"viewer", elem.Text("Viewer Section")},
)

With this approach, there's always a safe fallback to the default case.

However, this doesn't entirely address the issue of having a case for 'if this, otherwise nothing.' While we haven't found a perfect solution yet, introducing None() seems like a practical interim fix for many scenarios.

Please feel free to share any differing views or additional insights. The more discussion here the better to highlight things maybe not yet considered.

gedw99 commented 11 months ago

Is there a test case to look at ?

nikpivkin commented 11 months ago

Hi @whisk !

There is a similar function in solid-js. Show takes a predicate and returns an element (fallback optional argument):

<Show when={state.count > 0} fallback={<div>Loading...</div>}>
  <div>My Content</div>
</Show>

Regarding the switch-case. Have you looked at the lo package?

userRole := "editor"

content := elem.Div(
    nil, lo.Switch[string, elem.Node](userRole).
        Case("admin", elem.Text("Admin Dashboard")).
    Case("editor", elem.Text("Editor Tools")).
    Case("viewer", elem.Text("Viewer Section")).
    Default(elem.Text("Unknown Role")),
)
chasefleming commented 11 months ago

@nikpivkin The Show function in SolidJS did actually inspire what is now the If function in elem-go, and it was initially named Show as well.

Re the package, looking at it makes me wonder how much this library should even implement utility functions for things like conditional rendering vs recommend people use the utility libraries they like and let elem-go stick to its main function of creating HTML. Food for thought.

Separately for earlier in this thread, I've created an issue for the addition of the None() here: https://github.com/chasefleming/elem-go/issues/88