lambdaisland / hiccup

Enlive-backed Hiccup implementation (clj-only)
Mozilla Public License 2.0
28 stars 4 forks source link

Convert attributes to camelCase, with some exceptions #5

Closed rafd closed 1 year ago

rafd commented 1 year ago

When using Reagent+React, attributes are camelCased, ex. :foo-bar "baz" -> fooBar="baz".

...except for:

To enable better re-use of the same component between Reagent and on the backend, this PR implements converting from kebab-case to camelCase by default, except for the exceptions mentioned above.

Note: this does change existing behaviour, so it is "BREAKING".

Also: CHANGELOG.md has a placeholder message, but needs to be updated before merging into master.

plexus commented 1 year ago

If you use camelCase in your code, will that still work? If so then I think the risk of breaking existing code is small enough to risk it.

rafd commented 1 year ago

"camelCase" and :camelCase are untouched. Added a test.

alysbrooks commented 1 year ago

This will turn CamelCased attributes that are supposed to be kebab-cased into kebab-case, right?

I think the CHANGELOG entry should call out that this is a breaking change, although it should also point out we expect it to cause very little breakage and brings us inline with Reagent.

rafd commented 1 year ago

@alysbrooks No, not as written. But this is something we could do.

I took a fresh look at this issue:

Below are the various ways Reagent+React handles attributes (strings vs keywords, kebab vs camel), compared to this library's current output and this PRs current output. (Note: font-style is an attribute that HTML expects kebab-case, while tabindex is expected lowercase)

Screen Shot 2023-05-11 at 20 33 47

As is, this PR only addressed one of the above rows (:tab-index), and, incompletely at that (it should output tabindex, not tabIndex).

@plexus @alysbrooks Do you want this library to match what Reagent+React does in all cases? If so, I will make further changes.

alysbrooks commented 1 year ago

Hi @rafd, thanks for your thorough research!

I think there are a couple strategies we could pursue. One is matching React (and Reagent) behavior. Another is trying to produce standard output (e.g., tabindex instead of tabIndex since it's typically written lower case). A final one is avoiding unnecessary changes to user input (e.g. leaving tabIndex as is because there's no semantic difference between that and tabindex). The last two directly contradict each other, even though both could be seen as following the principle of least surprise. Producing standard output may reduce surprises for people (and programs) reading the output. Reducing (technically) unnecessary transformations may reduce surprises for people producing the output.

I'm inclined to favor React and Reagent behavior since that's a stated goal of the library, and it serves as a tiebreaker between the other two goals. @plexus do you have an opinion?

Incidentally this is the W3C HTML spec on case sensitivity:

In the HTML syntax, attribute names, even those for foreign elements, may be written with any mix of lower- and uppercase letters that are an ASCII case-insensitive match for the attribute's name.

And WHATWG:

A custom data attribute is an attribute in no namespace whose name starts with the string "data-", has at least one character after the hyphen, is XML-compatible, and contains no ASCII upper alphas.

Although they also say attributes in HTML are automatically lowercased, so this restriction doesn't affect HTML. (I think this means it only affects XML in practice?)

plexus commented 1 year ago

I haven't had a chance to look into this deeply, and I'm on my phone right now so I might have missed some of the finer points raised. I'll give some gebrek thoughts and will trust in folks doing the right thing.

Reagent compatibility is a goal on the syntax level, as much as is practical the same syntax should do the same thing in reagent as when and when generating static or server side html with this library. So the generated elements should be interpreted the same way in the browser in both cases.

For data attributes the most intuitive behavior imo is to preserve kebap casing, does that match reagent?

Thank you for working on this, it's ok to make small improvements where you see an opportunity. We don't need to fix everything all at once.

On Sat, May 20, 2023, 04:39 A Brooks @.***> wrote:

Hi @rafd https://github.com/rafd, thanks for your thorough research!

I think there are a couple strategies we could pursue. One is matching React (and Reagent) behavior. Another is trying to produce standard output (e.g., tabindex instead of tabIndex since it's typically written lower case). A final one is avoiding unnecessary changes to user input (e.g. leaving tabIndex as is because there's no semantic difference between that and tabindex). The last two directly contradict each other, even though both could be seen as following the principle of least surprise. Producing standard output may reduce surprises for people (and programs) reading the output. Reducing (technically) unnecessary transformations may reduce surprises for people producing the output.

I'm inclined to favor React and Reagent behavior since that's a stated goal of the library, and it serves as a tiebreaker between the other two goals. @plexus https://github.com/plexus do you have an opinion?

Incidentally this is the W3C HTML spec https://dev.w3.org/html5/spec-LC/syntax.html#attributes-0 on case sensitivity:

In the HTML syntax, attribute names, even those for foreign elements https://dev.w3.org/html5/spec-LC/syntax.html#foreign-elements, may be written with any mix of lower- and uppercase letters that are an ASCII case-insensitive https://dev.w3.org/html5/spec-LC/infrastructure.html#ascii-case-insensitive match for the attribute's name.

And WHATWG https://html.spec.whatwg.org/multipage/dom.html#embedding-custom-non-visible-data-with-the-data-*-attributes :

A custom data attribute is an attribute in no namespace whose name starts with the string "data-", has at least one character after the hyphen, is XML-compatible https://html.spec.whatwg.org/multipage/infrastructure.html#xml-compatible, and contains no ASCII upper alphas https://infra.spec.whatwg.org/#ascii-upper-alpha.

Although they also say attributes in HTML are automatically lowercased, so this restriction doesn't affect HTML. (I think this means it only affects XML in practice?)

— Reply to this email directly, view it on GitHub https://github.com/lambdaisland/hiccup/pull/5#issuecomment-1555436531, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAH3VGCW6U4FO2JVSI7JNTXHAVHFANCNFSM6AAAAAAX2HPUHE . You are receiving this because you were mentioned.Message ID: @.***>

alysbrooks commented 1 year ago

For data attributes the most intuitive behavior imo is to preserve kebap casing, does that match reagent?

Yes, it does, fortunately. The Reagent behavior seems to be pretty reasonable, although I'm not sure how I feel about the slightly difference in behavior between strings and attributes.

One case that I could see this actually tripping people up is using libraries like HTMX, which add a few custom attributes. HTMX uses the hx-* convention, and I believe after this change :hx-post would be converted to hxPost=, so users would have to use "hx-post" instead.

Hi @rafd, no rush on this, but are you planning to return to this soon? Like Arne mentioned, you can wrap up these changes for now and then come back to do the rest if you like.

rafd commented 1 year ago

My original issue was that :tab-index would convert differently in this library ("tab-index") vs. Reagent + React ("tabindex"). This patch as of https://github.com/lambdaisland/hiccup/pull/5/commits/f739a5097cd9d722557e185d0ea82c491b20af50 only addressed that one case (resulting in "tabIndex"`, which is good enough for me).

Re: upper vs lower case, IMO, it's not a big deal, b/c as mentioned, HTML is case-insensitive for attribute names.

Re: kebab-case vs camelCase and "strings" vs :keywords, the React+Reagent behaviour is non-trivial, but simple to implement. Incoming patch.

rafd commented 1 year ago

I've now updated the behaviour to match Reagent+React in all cases, except not forcing lowercasing.

I implemented it as two steps, to mimic the Reagent logic, and then the React logic.

Note:

alysbrooks commented 1 year ago

Sorry, one last thing: can you add some description to the CHANGELOG entry that this is a breaking change?

rafd commented 1 year ago

@alysbrooks made the changes.

For posterity, here is what the conversions look like now:

Screen Shot 2023-06-13 at 18 35 50
alysbrooks commented 1 year ago

Thanks, @rafd! Animation of a green cartoon bean doffing its orange hat with "thanks!" written underneath