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

exempt strings from being treated as known booleans unless empty or equal to name #46

Merged
merged 2 commits into from
Dec 13, 2024

Conversation

hesxenon
Copy link
Contributor

@hesxenon hesxenon commented Dec 8, 2024

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and discussions and couldn’t find anything or linked relevant results below
  • I made sure the docs are up to date
  • I included tests (or that’s not needed)

Description of changes

This change prevents treating string attributes whose name is a well known boolean (or overloaded boolean) as a boolean if their values are

  • not empty
  • not equal to their attributes name

This is done to enable e.g. custom element to re-use those well known names with values that differ from their well known counterparts.

closes #45

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Dec 8, 2024
@hesxenon
Copy link
Contributor Author

hesxenon commented Dec 8, 2024

@wooorm here you go, I've added a TODO comment above an existing test as it appears to me that the behavior in question is not justified and it necessitates additional logic that could otherwise be dropped.

@@ -140,6 +140,7 @@ test('`element` attributes', async (t) => {
}
)

// TODO: why? "hidden" is a valid value for the `hidden` attribute
Copy link
Member

Choose a reason for hiding this comment

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

Because hidden looks better? Because hidden="hidden" is longer and superfluous? Because it’s how people often write HTML?

This package is also used in combination of minifiers / pritifyers. So it’s not solely about what “works”.

🤔 I’d say this comment can be removed?

@hesxenon
Copy link
Contributor Author

hesxenon commented Dec 9, 2024 via email

@wooorm
Copy link
Member

wooorm commented Dec 9, 2024

On why: because the tools are all made to help people that work with ASTs (plugins). There’s a line between CSTs and ASTs. ASTs are easier to work with.

You could argue that it doesn’t need to happen here, we can have that conversation: there might indeed be no need.

But this specific argument about differentiating between hidden="hidden" and hidden="" and hidden affects the entire ecosystem but also CSS and JavaScript. HTML specifies that there are different ways to write boolean attributes. And that hidden is a global attribute. I think the custom element author is wrong to differentiate between these values, and should instead accept all.

@hesxenon
Copy link
Contributor Author

Hmm, in the case of hidden I agree. Not sure I understand your point about CST vs. AST exactly - is it about the following?

If a CE author attaches different semantics to well known attributes (that are not defined as global) transforming that correctly requires a context (namely the context of the CE) which a CST util would care about, but an AST transformer does not because it sort of does not care about other leafs and branches of the tree? If so, then why does this AST transformer care about property information at all? Shouldn't it just take the given value and attach it unchanged?

Anyway, I'll remove the comment so this can be merged, would be happy to learn more about this though, if you have any pointers or such.

@wooorm
Copy link
Member

wooorm commented Dec 13, 2024

So a CST cares about everything. "asd" and 'asd' are different to it. An AST does not care: they are equivalent strings, the quotes don’t matter. Or, there is a lot of insignificant whitespace.
However, a user could write a script that checks someFunction.toString(), for its length, or for which quotes are in there.
For HTML, a CST would indeed differentiate between hidden="hidden", hidden='hidden', hidden=hidden, hidden="", hidden. Or also, typically insignificant whitespace. Or, say, in HTML you can write two <body> opening tags with different attributes, a CST would keep them separate, but according to the HTML parsing rules they will be merged onto one <body> with all those attributes.

const d = document.createElement('div')
d.innerHTML = '<div hidden="asd">'
d.children[0].getAttribute('hidden') // 'asd'
d.children[0].hidden // true
d.innerHTML // '<div hidden="asd"></div>'

d.innerHTML = '<d-v hidden="asd">'
d.children[0].getAttribute('hidden') // 'asd'
d.children[0].hidden // true
d.innerHTML // '<d-v hidden="asd"></d-v>'

Some parts of HTML persist things, some don’t.

Persist all information makes it rather hard to work with. Dropping info makes it easier to work with.

Dropping whether an original HTML author wrote hidden='hidden' or hidden=hidden or hidden, but representing it only as properties.hidden: boolean, makes things simpler for AST users.

A JS user could differentiate with someFunction.toString().
An HTML user (with CSS or client side JS), could differentiate things. Whether it’s custom elements or a regular div. What is useful/useless is not an exact line. But I’d say typically CSS/client-side JS users would treat hidden as a boolean property instead of looking for its exact value.

@wooorm wooorm merged commit 6000808 into syntax-tree:main Dec 13, 2024
2 checks passed
@wooorm wooorm added the 💪 phase/solved Post is done label Dec 13, 2024

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Dec 13, 2024
@wooorm
Copy link
Member

wooorm commented Dec 13, 2024

Thanks, released in 9.0.4!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

Cannot use well-known attribute names with custom elements
2 participants