Skip to content

Convert attributes to camelCase, with some exceptions #5

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

Merged
merged 7 commits into from
Jun 16, 2023
Merged

Convert attributes to camelCase, with some exceptions #5

merged 7 commits into from
Jun 16, 2023

Conversation

rafd
Copy link
Contributor

@rafd rafd commented May 8, 2023

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

...except for:

  • data-* and aria-* attributes (reagent src)
  • strings (reagent src)
  • some HTML and SVG attributes (accept-charset, accent-height, and many others; React expects them camelCased, but converts to kebab-case when rendering.)

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

plexus commented May 8, 2023

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

rafd commented May 9, 2023

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

@alysbrooks
Copy link
Member

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

rafd commented May 12, 2023

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

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

plexus commented May 22, 2023 via email

@alysbrooks
Copy link
Member

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

rafd commented May 30, 2023

My original issue was that :tab-index would convert differently in this library ("tab-index") vs. Reagent + React ("tabindex"). This patch as of f739a50 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
Copy link
Contributor Author

rafd commented May 30, 2023

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:

  • all attributes get converted to strings (only relevant to downstream post-processing, which I changed for the :style case)
  • the existing tests are written in (is (= actual expected)) style, which resulted in confusing test output; but I wrote the new tests in (is (= expected actual)) style. I could update to conform.

@alysbrooks
Copy link
Member

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

@rafd
Copy link
Contributor Author

rafd commented Jun 13, 2023

@alysbrooks made the changes.

For posterity, here is what the conversions look like now:
Screen Shot 2023-06-13 at 18 35 50

@alysbrooks alysbrooks merged commit 7c4925e into lambdaisland:main Jun 16, 2023
@alysbrooks
Copy link
Member

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

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.

3 participants