Skip to content

Less case modifications for keys #1030

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Less case modifications for keys #1030

wants to merge 3 commits into from

Conversation

gazpachoking
Copy link
Contributor

@gazpachoking gazpachoking commented Jul 30, 2025 •
edited
Loading

New try at #1025

Main ideas:

  • Leave the data-* attribute key in the original (attribute) casing when passing to applyAttributeFunction and attribute plugins. This makes it easier to reason about splitting on dashes and future modifications.
  • Don't do any case modification until the last minute. Case mods don't do any sanitizing. They just do the minimum to get you from the original case to what you desire.
  • I didn't change the behavior of the regular 'camel', 'kebab', 'snake' functions this time, because they are also being used outside of attribute key modification. I think those could be simpler as well, but am less confident about the state of the inputs going in to them.

Things I actually think are bad with the current implementation (that this PR fixes):

  • data-signals-letter4number ends up capitalizing the first letter after a number. (letter4Number) Very surprising, and no case modifier that can save you at the moment.
    • If you use __case.kebab with the above it keeps things lower, but adds dashes around the number. This seems less bad, but I see no need to add magic dashes when you can just data-signals-letter-4-number__case.kebab if that was your intention. kebab in this PR just keeps literally whatever you had in the attribute.
  • Can't start a key with a number. data-signals-1foo Just doesn't work right now. Don't see why it shouldn't.

Things that aren't really bad but this fixes anyway:

  • An alias with a dash in. An alias that starts with a non-letter.
  • Starting a key with a dash. e.g. data-style---css-var
  • Double dashes. data-signals-my--doubledash__case.kebab

Things this will 'break':

  • If you were relying on datastar to change your key name in ways you could have just changed your key name yourself. e.g. Adding dashes for you. Removing extra dashes for you. Capitalizing things that didn't have a dash before them.

gazpachoking
gazpachoking commented Jul 30, 2025
View reviewed changes
library/src/engine/engine.ts
if (!el.closest(ignore)) {
for (const key in el.dataset) {
applyAttributePlugin(el, key, el.dataset[key]!)
applyAttributePlugin(el, key.replace(/[A-Z]/g, '-$&').toLowerCase(), el.dataset[key]!)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just undo dataset case.

library/src/utils/text.ts
export function modifyCasing(str: string, mods: Modifiers) {
for (const c of mods.get('case') || []) {
export function modifyCasing(str: string, mods: Modifiers, defaultCase: string = 'camel') {
for (const c of mods.get('case') || [defaultCase]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes the default case a param so that only one case modifier is applied. Before the default case would be applied no matter what, then the user case applied on top of that.

library/src/plugins/backend/watchers/patchElements.ts Outdated
for (const { name, value } of (newNode as Element).attributes) {
if (
(oldNode as Element).getAttribute(name) !== value &&
!preserveAttrs.includes(kebab(name))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why we are applying kebab case to raw element attribute names here. Perhaps I shouldn't have changed this though because it's not related to my other changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took this change out. Originally I was trying to remove all the non attribute key uses of the case functions, but I ended up leaving them untouched and creating separate ones for case mods.

@bencroker
Copy link
Collaborator

bencroker commented Oct 4, 2025 •
edited
Loading

Thanks for the PR, @gazpachoking!

We’re planning on switching - with : as the delimiter between plugin name and key (data-on-click → data-on:click), meaning that this PR will need to be reworked. You definitely have good ideas in here, so perhaps @jmstevers can take a look and we can use them for RC.6, otherwise I may end up asking you to create a new version after RC.6 is released.

@gazpachoking
Copy link
Contributor Author

Updating for colons was like a 2 character change, so I went ahead and did that and resolved conflicts. Happy to do it again on top of whatever changes you guys have internally though if things have diverged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@delaneyj delaneyj Awaiting requested review from delaneyj delaneyj is a code owner

@bencroker bencroker Awaiting requested review from bencroker bencroker is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@gazpachoking @bencroker