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

Spread attributes #237

Merged
merged 16 commits into from
Dec 21, 2023
Merged

Spread attributes #237

merged 16 commits into from
Dec 21, 2023

Conversation

gungun974
Copy link
Contributor

@gungun974 gungun974 commented Oct 17, 2023

Introduction

This Pull Request was created to address the primary issue of #229.

The Templ component system prevents passing attributes directly to an element within the component.

This can be quite problematic as it complicates the use of libraries like HTMX, alpinejs, or hyperscript due to an almost infinite number of attributes.

This Pull Request adds a "spread" attribute behavior to Templ similar to what can be found in React.

The "spread attributes" syntax

"Spread attributes" are a new type of attribute that allows adding directly to the generated code a map[string]string as a list of an element's attributes.

For this, the Templ generator detects the "spread attributes" syntax and adds to the code a for range of this map and escapes the name and the value of the attribute at runtime.

An alias templ.Attributes is available in the runtime to enhance visibility and avoid writing map[string]string.

Here's an example of code written with this syntax.

package spread_attributes

templ AppButton(attrs templ.Attributes) {
  <button { attrs... } />
}

templ Consume() {
  @AppButton(templ.Attributes{"data-id": "my-custom-id"}) {
    Click
  }
}

This syntax was inspired by the proposal from @joerdav.

Todo

This Pull Request is not yet complete; here are the missing points:

  • Recognize the "spread attributes" syntax
  • Generate runtime code handling the "spread attributes"
  • Update the documentation to explain the operation of "spread attributes"
    [ ] (Not sure) Overwrite conflicting attributes (This could have a higher runtime impact as this would break constant attributes with the current operation) (It's not my responsibility in the end)

@xgalaxy
Copy link

xgalaxy commented Oct 17, 2023

I'm not as familiar with the code as you are. But you might want to make sure this doesn't clash with children...

It looks like the parser specifically looks for children... so there might not be an issue. I don't know if templ throws a parsing error if a variable coming in contains the name children or not so maybe a potential clash is already impossible?

@gungun974
Copy link
Contributor Author

I'm not as familiar with the code as you are. But you might want to make sure this doesn't clash with children...

It looks like the parser specifically looks for children... so there might not be an issue. I don't know if templ throws a parsing error if a variable coming in contains the name children or not so maybe a potential clash is already impossible?

This is not an issue, the parser is divided into smaller parsers, and each smaller parser has a higher priority than the other.

So here we have the element.Parse parser which is executed before childrenExpression.Parse.

So there isn't really any conflict. Moreover, it's important to understand that the { children... } for injecting a child is not real.
If you look at the generated template, you won't actually see a "children" variable.
What the generator does is that it generates code that fetches the child from the runtime context and calls the render method.

However, for safety, in case the order changes one day, I will modify my unit tests. ^^

@gungun974 gungun974 marked this pull request as ready for review October 18, 2023 07:51
Copy link
Collaborator

@joerdav joerdav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome PR!

docs/docs/03-syntax-and-usage/03-attributes.md Outdated Show resolved Hide resolved
docs/docs/03-syntax-and-usage/03-attributes.md Outdated Show resolved Hide resolved
parser/v2/elementparser.go Outdated Show resolved Hide resolved
}

// Expression.
if attr.Expression, ok, err = exp.Parse(pi); err != nil || !ok {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential use case for a parser.Until, will prevent you from having to trim as below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may not work in this case actually. But worth thinking about!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential use case for a parser.Until, will prevent you from having to trim as below.

This can't work here because the expression parser grab the three dots.

The only thing I can do is try to change the "expression parser" but the issue is if I remove the ellipses from the value.

I can't after in the spreadAttributesParser check if In my expression I got my ellipses since I need to look into the past.

I think for now the simplest thing is as it its now. (Note, I try things before saying that)

docs/docs/03-syntax-and-usage/03-attributes.md Outdated Show resolved Hide resolved
@joerdav
Copy link
Collaborator

joerdav commented Oct 18, 2023

It looks like due to map semantics, the attributes are appearing in a random order. Either need to loop through in an ordered way or change to a slice

type Attribute struct {
	Key, Value string
}
type Attributes []Attribute

The usage wouldn't be too different if it was moved to a slice:

	component := BasicTemplate(templ.Attributes{
		{"id",     "test"},
		{"hx-get", "/page"},
	})

@gungun974
Copy link
Contributor Author

It looks like due to map semantics, the attributes are appearing in a random order. Either need to loop through in an ordered way or change to a slice

type Attribute struct {
	Key, Value string
}
type Attributes []Attribute

The usage wouldn't be too different if it was moved to a slice:

	component := BasicTemplate(templ.Attributes{
		{"id",     "test"},
		{"hx-get", "/page"},
	})

Yes, I was going to say that I had forgotten that Go randomly randomizes the maps and therefore that it breaks the tests.

So in HTML it's not a problem but it's annoying.

I personally think it would be better to don't change the signature and just make the runtime insert attribute by alphabetical order.

@gungun974
Copy link
Contributor Author

I had already written this for a personal project:

sortedAttributes := make([]string, 0, len(attributes))

for k := range attributes {
    sortedAttributes = append(sortedAttributes, k)
}

sort.Strings(sortedAttributes)

for _, k := range sortedAttributes {
    v := attributes[k]
}

I don't think this will have such a negative impact on performance and at least we are sure of the order.

@gungun974
Copy link
Contributor Author

gungun974 commented Oct 18, 2023

It looks like due to map semantics, the attributes are appearing in a random order. Either need to loop through in an ordered way or change to a slice

type Attribute struct {
	Key, Value string
}
type Attributes []Attribute

The usage wouldn't be too different if it was moved to a slice:

	component := BasicTemplate(templ.Attributes{
		{"id",     "test"},
		{"hx-get", "/page"},
	})

Also now I think using slice could make some runtime mistake if someone create a slice with only the key and not a value or strange case when someone create an attribute with a key, a value and something else.

component := BasicTemplate(templ.Attributes{
  {"id"},
  {"hx-get", "/page", "third"},
})

Map prevent those case.

@joerdav
Copy link
Collaborator

joerdav commented Oct 18, 2023

@gungun974 That would not be the case:

https://go.dev/play/p/-wX7DtB9JrN

@joerdav
Copy link
Collaborator

joerdav commented Oct 18, 2023

Yep, don't think performance would be too much of an issue either, these maps should be relatively small.

@gungun974
Copy link
Contributor Author

@gungun974 That would not be the case:

https://go.dev/play/p/-wX7DtB9JrN

Oh you make me learn something.
I still prefer sort map since map got some extra method link to them but yes, it's not all that bad.

Copy link
Collaborator

@joerdav joerdav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good, will need @a-h to take a look and decide if this is the API we want to go with.

My only reservation is that the ... syntax in Go is for slices/arrays rather than maps, but maybe that is okay.

@gungun974
Copy link
Contributor Author

My only reservation is that the ... syntax in Go is for slices/arrays rather than maps, but maybe that is okay.

I can understand ... can be strange since we give here an another meaning.

But to argue with this syntax. Templ already use children... to insert child element and those ellipse or not really common in go (there are not spread operator used everywhere like JS 💀).


This all looks good, will need @a-h to take a look and decide if this is the API we want to go with.

If you (@joerdav) or @a-h want to change the way the parser represent that new attribute type or the way the generator generate.

I'm willing of course to improve this PR ^^

@a-h
Copy link
Owner

a-h commented Dec 11, 2023

Sorry for not merging this. Still not sure I've thought through all of the consequences, and want to get the design right.

I was thinking about how you would send things like optional attributes down the chain.

There are also boolean attributes that don't have a key/value pair, e.g. noshade. They just appear in HTML.

I thought the design might need to be more than a map[string]string, and be a map[string]Attribute to accommodate that.

package spread_attributes

type Attribute struct {
  Name string
  Value string
  OmitValue bool // https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes
  Visible func(ctx context.Context) bool
}

templ AppButton(attrs templ.Attributes) {
  <button { attrs... } />
}

type AttrOpt func(*templ.Attributes)

func ConditionalAttr(condition bool, name, value string) AttrOpt {
  return func(a *templ.Attributes) {
      a[name] = value
      a[name].Visible = func(ctx context.Context) bool { return condition }
  }
}

func Attr(name, value string) AttrOpt {
  return func(a *templ.Attributes) {
    a[name] = value
  }
}

func BooleanAttr(name string) AttrOpt {
  return func(a *templ.Attributes) {
    a[name].OmitValue = true
  }
}

func Attributes(opts ...AttrOpt) templ.Attributes {
  a := make(templ.Attributes)
  for _, opt := range opts {
    opt(a)
  }
  return a
}

templ Consume() {
  @AppButton(Attributes(BooleanAttr("noshade"), ConditionalAttr(isLoggedIn, "class", "authenticated"), Attr("data-id": "my-custom-id"))) {
    Click
  }
}

Or am I overthinking this one?

@gungun974
Copy link
Contributor Author

gungun974 commented Dec 11, 2023

So it's complicated for me to say if you "overthink" since I'm biased about my syntax but looking with the spec of HTML boolean attributes you have shown.

https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

It's written : If the attribute is present, its value must either be the empty string or a value that is an ASCII case-insensitive match for the attribute's canonical name, with no leading or trailing whitespace.

That mean in HTML you can write a disabled attribute with an empty value or the disabled value. In fact I will don't lie to you but I never write the shorten version without the value been the same 😅

So for the example of noshade nothing prevent you to use the longer syntax. Of course if Templ try to aim to the less HTML generate it's more verbose but I think HTML is cheap with gzip so 6 characters more is not the end of the world.


Otherwise I think this Syntax can be Interesting. I think with some formatting it can be regular to read

templ Consume() {
  @AppButton(
    Attributes(
      BooleanAttr("noshade"),
      ConditionalAttr(isLoggedIn, "class", "authenticated"),
      Attr("data-id": "my-custom-id"),
    )
  ) {
    Click
  }
}

My only issue is that make the user need to learn a lot of custom structure just to do at the end a spread of attributes 👀
(Oh when originally saying that I didn't really read what does the structure but I think in term of pure "performance" it's worst than a map)

When you said to someone it's just a map[string]string it's easy to understand, as a smaller footprint when writing and since it's just a well know type in go, You know how write function to manipulate.

The only thing this I see in this Syntax that is better from my current PR is ConditionalAttr. With a map you can't use ternary (in fact we don't have ternary).

This can be very annoying since you have to create a helper function returning the write value. But I think this feature can be implemented if Templ has a way to define go code directly in the template. You could just define early the map, do the if append and then use it.

I think you (@a-h) mention early in an other Issue you planed to add this native go code injection in Templ. This feature can really help to simplify the user and the codebase of this project itself since it's give to the develop the power of writing code that solve they specific problem than trying to create some helper or complex structure like here.

Anyway that my take on this problem and I could be very wrong but I think it's the simpler and more go user friendly we can do for now.

@cmar0027
Copy link

cmar0027 commented Dec 11, 2023

Or am I overthinking this one?

Sorry for the intrusion, but what about just having type templ.Attributes = map[string]interface{}, then generate code that uses type assertion like this:

  • if the value is a string just add the attribute with its value
  • if the value is a bool, add the attribute as a boolean attribute only if value is true
  • if the value is a templ.KeyValue[string, bool], this would be an optional attribute
templ AppButton(attrs templ.Attributes) {
  <button { attrs... } />
}

templ Consume() {
  @AppButton(templ.Attributes{
    "nonshade": true,
    "class": templ.KV("authenticated", isLoggedIn),
    "dateId": "my-custom-id",
  }) {
    Click
  }
}

I don't know the templ code base well enough to say what approach would be faster, but this declarative one looks more intuitive to me.

@gungun974
Copy link
Contributor Author

Or am I overthinking this one?

Sorry for the intrusion, but what about just having type templ.Attributes = map[string]interface{}, then generate code that uses type assertion like this:

* if the value is a string just add the attribute with its value

* if the value is a bool, add the attribute as a boolean attribute only if value is true

* if the value is a templ.KeyValue[string, bool], this would be an optional attribute
templ AppButton(attrs templ.Attributes) {
  <button { attrs... } />
}

templ Consume() {
  @AppButton(templ.Attributes{
    "nonshade": true,
    "class": templ.KV("authenticated", isLoggedIn),
    "dateId": "my-custom-id",
  }) {
    Click
  }
}

Oh that's a great Idea I love it ! ❤️

That would mean the developer can pass everything and in the runtime we only easily ignore the attribute if the value is not valid (for example if it's was a float).

Also the fact we reused templ.KV for conditional is Neat !

I don't know the templ code base to say what approach would be faster, but this declarative looks more intuitive to me.

Of course the loop will be slower since we need to cast if the interface{} is a string, boolean or templ.KV but I don't think it's the kind of operation is very that slow.

The thing that hype me the most it's the kind of little change you suggest @cmar0027 that's so clean and so perfect that we couldn't see at first gland with our eyes so deep in complex solution !

@filipweidemann
Copy link

Hey, quickly chiming in:

I would double down on the point that @gungun974 made. We can easily write attribute values on e.g. hidden or others.

Also, @cmar0027's syntax is great imho! With this, there is no need to learn all the new attribute structs.

@gungun974
Copy link
Contributor Author

Hum CI/CD failed, I guess I will need to rebase this branch and verify everything still work.

For now 75ea299 add what we have discuss yesterday.

I didn't have yet update the documentation since I'm too bad to write english and explain thing.

I will fix later this day CI/CD but I hope the new code I wrote will be at your standard @a-h

@gungun974
Copy link
Contributor Author

Well I don't know what happen but there is something wrong with the CI/CD for me ???

I rebase with main and CI/CD still failed. So since I use NixOS I try to run the flake nix develop --command xc lint on my computer and got similar error from the CI/CD :

cmd/templ/generatecmd/sse/server.go:8:2: could not import sync/atomic (-: # sync/atomic
compile: version "go1.21.4" does not match go tool version "go1.21.1") (typecheck)
        "sync/atomic"
        ^
cmd/templ/visualize/types.go:23:13: undefined: parser (typecheck)
        sourceMap *parser.SourceMap
                   ^
cmd/templ/visualize/types.go:57:13: undefined: parser (typecheck)
        sourceMap *parser.SourceMap
                   ^
cmd/templ/visualize/types.go:15:78: undefined: parser (typecheck)
func HTML(templFileName string, templContents, goContents string, sourceMap *parser.SourceMap) templ.Component {
                                                                             ^
cmd/templ/generatecmd/main.go:128:8: undefined: backoff (typecheck)
        bo := backoff.NewExponentialBackOff()
              ^
cmd/templ/generatecmd/main.go:250:13: undefined: backoff (typecheck)
        backoff := backoff.NewExponentialBackOff()
                   ^
parser/v1/parser.go:6:2: could not import unicode (-: # unicode
compile: version "go1.21.4" does not match go tool version "go1.21.1") (typecheck)
        "unicode"
        ^
parser/v2/cssparser.go:5:2: could not import unicode (-: # unicode
compile: version "go1.21.4" does not match go tool version "go1.21.1") (typecheck)
        "unicode"
        ^
parser/v2/calltemplateparser.go:25:18: cannot infer T (parser/v2/expressionparser.go:16:11) (typecheck)
        if _, ok, err = Must(closeBraceWithOptionalPadding, "call template expression: missing closing brace").Parse(pi); err != nil || !ok {
                        ^
parser/v2/conditionalattributeparser.go:17:29: cannot infer T (parser/v2/expressionparser.go:16:11) (typecheck)
        if r.Expression, ok, err = Must(ExpressionOf(parse.StringUntil(parse.All(openBraceWithOptionalPadding, parse.NewLine))), "attribute if: unterminated (missing closing '{\n')").Parse(pi); err != nil || !ok {
                                   ^
parser/v2/conditionalattributeparser.go:22:18: cannot infer T (parser/v2/expressionparser.go:16:11) (typecheck)
        if _, ok, err = Must(parse.All(openBraceWithOptionalPadding, parse.NewLine), "attribute if: unterminated (missing closing '{')").Parse(pi); err != nil || !ok {
                        ^
parser/v2/forexpressionparser.go:29:9: cannot infer TUntil (parser/v2/templateparser.go:54:28) (typecheck)
        tnp := newTemplateNodeParser(closeBraceWithOptionalPadding, "for expression closing brace")
               ^
parser/v2/ifexpressionparser.go:32:40: cannot infer T (parser/v2/expressionparser.go:10:16) (typecheck)
        np := newTemplateNodeParser(parse.Any(StripType(elseIfExpression), StripType(elseExpression), StripType(closeBraceWithOptionalPadding)), "else expression or closing brace")
                                              ^
parser/v2/ifexpressionparser.go:85:40: cannot infer T (parser/v2/expressionparser.go:10:16) (typecheck)
        np := newTemplateNodeParser(parse.Any(StripType(elseIfExpression), StripType(elseExpression), StripType(closeBraceWithOptionalPadding)), "else expression or closing brace")
                                              ^
parser/v2/ifexpressionparser.go:115:18: cannot infer TUntil (parser/v2/templateparser.go:54:28) (typecheck)
        if r, ok, err = newTemplateNodeParser(closeBraceWithOptionalPadding, "else expression closing brace").Parse(in); err != nil || !ok {
                        ^
parser/v2/parser.go:22:37: cannot infer TUntil (parser/v2/templateparser.go:54:28) (typecheck)
        r.Children, ok, err = Must[[]Node](newTemplateNodeParser(closeBraceWithOptionalPadding, "template closing brace"), "templ: expected nodes in templ body, but found none").Parse(pi)
                                           ^
parser/v2/switchexpressionparser.go:82:40: cannot infer T (parser/v2/expressionparser.go:10:16) (typecheck)
        pr := newTemplateNodeParser(parse.Any(StripType(closeBraceWithOptionalPadding), StripType(caseExpressionStartParser)), "closing brace or case expression")

But using act with act -j lint to run GitHub Actions locally don't yield error ???

And finally updating the flake.lock solve my error with xc lint ???

And the worst is those strange behavior I can reproduce on the main branch in a fresh clone repo...

I don't know what happen to me but I don't think it's my fault :(

@a-h
Copy link
Owner

a-h commented Dec 12, 2023

Don't worry, I'll check out what's going on with the linter.

@imaai
Copy link

imaai commented Dec 17, 2023

@a-h @gungun974 Any updates on this one?

@a-h
Copy link
Owner

a-h commented Dec 21, 2023

OK, I've reworked this PR and I think the end result is a bit simpler.

@a-h a-h merged commit 37f022a into a-h:main Dec 21, 2023
3 of 4 checks passed
@filipweidemann
Copy link

You have no idea how happy I am right now!
It really was a showstopped for me when trying to migrate an existing JS-based app to Go & templ, so with this PR I have all the (obvious) bits I need to pull this off.

Thank you so much for your efforts @a-h, @gungun974 & everyone who contributed by either reviewing or shaping the feature otherwise!

@alehechka alehechka mentioned this pull request Jan 18, 2024
@the-goodies
Copy link

the-goodies commented Feb 25, 2024

Am I misunderstanding something or you can't pass a single attribute and use it like: { attr }?
What is the point of templ.Attributes being map[string]any instead of []Attribute where Attribute is struct{Key string, Value any}? If it was a struct, you could pass a single value or pass a slice and use spread operation which works on slices in Go, unlike map which doesn't. This further adds, while minor, but unnecessary differences between templ and Go for no apparent reason.

A new slog package has Attr struct, templ should have had a similar one, instead of being map[string]any:

type Attr struct {
	Key    string
	Value Value // more or less type any
}

@filipweidemann
Copy link

Hey, I'm no maintainer or even contributor (yet), but I want to quickly ask how the current implementation prevents you from achieving your goal here?

I am not even sure if having []Attr would solve any real and existing issue. Is there something preventing you from using templ.Attributes{"my-key", "my-value"} & { attrs... }? Is there a specific use case you currently cannot implement with this feature in its current state? You should absolutely be able pass single attributes using this syntax.

As for the syntax decisions: the feature is consistent with existing templ functionality, e.g. what @a-h calls child spread expressions

I would argue that consistency inside the library itself is more important than parity with the underlying language.
People use JSX after all. I would not call that HTML, nor JS. It is an extensions for JS for templating (primarily) React components (just like templ is for templating HTML using Go) & gets compiled to proper code. Same story if you'd ask me.

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.

8 participants