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

Omitting else clause in If method #67

Open
whisk opened this issue Nov 7, 2023 · 11 comments
Open

Omitting else clause in If method #67

whisk opened this issue Nov 7, 2023 · 11 comments

Comments

@whisk
Copy link
Contributor

whisk commented Nov 7, 2023

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
Copy link
Owner

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

  • Turn If into IfElse (unfortunately, introducing a breaking change) and then just introduce If.
  • Allow If to take nil as a ifFalse value so you could do If(shouldWeShowLink, link, nil).

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
Copy link

gedw99 commented Nov 8, 2023

@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
Copy link
Owner

chasefleming commented Nov 8, 2023

@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
Copy link
Owner

@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
Copy link

gedw99 commented Nov 8, 2023

I say go with whatever works now.

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

@whisk
Copy link
Contributor Author

whisk commented Nov 11, 2023

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
Copy link

gedw99 commented Nov 11, 2023

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
Copy link
Owner

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
Copy link

gedw99 commented Nov 18, 2023

Is there a test case to look at ?

@nikpivkin
Copy link

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
Copy link
Owner

chasefleming commented Nov 22, 2023

@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: #88

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

No branches or pull requests

4 participants