fix: replace HTML_LOWER_CASE regex with Set to prevent false matches …#452
fix: replace HTML_LOWER_CASE regex with Set to prevent false matches …#452baspinarenes wants to merge 4 commits intopreactjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 04e12ea The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…on custom elements The regex could incorrectly match custom element properties (e.g. channelId matching the `ch` prefix, resulting in `channelid`). Using an explicit Set ensures only known HTML attributes are lowercased. Fixes preactjs#451
baspinarenes
force-pushed
the
fix/html-lower-case-set
branch
from
2de1537 to
cff3239
Compare
src/pretty.js
Outdated
| : name.replace(/([A-Z])/g, '-$1').toLowerCase(); | ||
| } | ||
| } else if (HTML_LOWER_CASE.test(name)) { | ||
| } else if (HTML_LOWER_CASE.has(name)) { |
There was a problem hiding this comment.
Should we instead check whether this is a custom-element by doing isCustomElement && .test to avoid breakages because of forgetting properties in the Set
There was a problem hiding this comment.
Though, I supposed it'd also be a breaking change if a custom element used any of the aforementioned properties, expecting them to be serialized as HTML attrs.
I don't have any immediate answer, just thinking out loud
There was a problem hiding this comment.
thanks for the feedbacks. i refactored it to work specifically with the native element 🙏
There was a problem hiding this comment.
keeping static sets seems like the safest method. it also seems like we can't do it through a property that only exists in native elements. i'm open to different ideas 🙏
There was a problem hiding this comment.
You did the inverse, we mainly want to align the logic. I.e. keep the regex and check whether or not you are dealing with a custom element.
Ref: https://github.com/preactjs/preact/blob/main/compat/src/render.js#L129
There was a problem hiding this comment.
we have a microfrontend framework in our company, and here we store pages as JSX. we use a custom fragment named <Fragment to retrieve microfrontend components. we also render this JSX file as HTML (ssr render output) using preact-render-to-string (<Fragment -> <fragment). that's what I meant by custom element. when we check with a hyphen, we end up ignoring these cases. do you think the package should consider these cases?
There was a problem hiding this comment.
btw thanks for the guidance. this is my first open source contribution 😄
There was a problem hiding this comment.
that's what I meant by custom element
It sounds like you're actually referring to a component, going by the casing. Custom elements are something fairly different and must always have a hyphen in their names.
There was a problem hiding this comment.
Yea, no, we should not support non-standard html tags that aren't custom elements. I would not merge that
3 participants
Fixes #451
Summary
HTML_LOWER_CASEregex with an explicitSetof known camelCase HTML attribute names.test()calls to.has()in bothsrc/index.jsandsrc/pretty.jsProblem
The regex
^(?:accessK|auto[A-Z]|cell|ch|col|...)uses broad prefixes that can falsely match custom element properties. For example,channelIdmatches thechprefix (intended forcharSet) and gets incorrectly lowercased tochannelid.Solution
Replace the regex with an explicit
Setso only known HTML attributes are lowercased. Custom element properties are no longer affected.