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.

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

fix(jsx): props are set as properties #999

Draft
wants to merge 1 commit into
base: v3
Choose a base branch
from
Draft

fix(jsx): props are set as properties #999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/jsx/src/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,12 +390,12 @@ it('same arguments in ref mount and unmount hooks', setup(async (ctx, h, hf, mou
assert.is(unmountArgs[1], component)
}))

it('css property and class attribute', setup(async (ctx, h, hf, mount, parent) => {
it('css property and className property', setup(async (ctx, h, hf, mount, parent) => {
const cls = 'class'
const css = 'color: red;'

const ref1 = (<div css={css} class={cls}></div>)
const ref2 = (<div class={cls} css={css}></div>)
const ref1 = (<div css={css} className={cls}></div>)
const ref2 = (<div className={cls} css={css}></div>)

const component = (
<div>
Expand Down Expand Up @@ -445,7 +445,7 @@ it('css custom property', setup(async (ctx, h, hf, mount, parent) => {
assert.is(component.style.getPropertyValue('--secondProperty'), '')
}))

it('class and className attribute', setup(async (ctx, h, hf, mount, parent) => {
Copy link
Owner

Choose a reason for hiding this comment

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

We need to stay a special case for class and className, it should work without prefixes in both ways.

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 suggest keeping only className:

  • Supporting only one way to assign classes makes the library's API simpler. If two options (class and className) are available, it can cause confusion and increase the likelihood of errors, such as attempting to use both properties simultaneously.
  • Simplification of internal logic. It eliminates the need to decide which property takes precedence in case of a conflict.
  • Most developers working with JSX are already accustomed to using className. Adding support for class may introduce unnecessary inconsistencies and complicate transitions between projects.
  • Including class increases mental overhead by introducing an exception. It is much easier to understand that a property in JSX corresponds directly to an element's property.

Copy link
Owner

@artalar artalar Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There are a valid points, but it are all controversial.

  • there is no big API overhead for only this special case
  • many developers which didn't know JSX specific may be confused, there are a lot of templates engines (including Preact!) which support "class"
  • I think most part of developers doesn't know \ understand \ remember a difference between attributes and properties.

And also, the change that you want to do it a breaking change and a new major release - it's not worth it

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 looked at the preact source code, came across the exception handling and moved it to reatom/jsx
https://github.com/preactjs/preact/blob/main/src/diff/children.js#L101

it('class and className properties', setup(async (ctx, h, hf, mount, parent) => {
const classAtom = atom('' as string | undefined)

const ref1 = (<div class={classAtom}></div>)
Expand Down Expand Up @@ -473,8 +473,8 @@ it('class and className attribute', setup(async (ctx, h, hf, mount, parent) => {
classAtom(ctx, undefined)
assert.is(ref1.className, '')
assert.is(ref2.className, '')
assert.ok(!ref1.hasAttribute('class'))
assert.ok(!ref2.hasAttribute('class'))
assert.ok(ref1.hasAttribute('class'))
assert.ok(ref2.hasAttribute('class'))
}))

it('ref mount and unmount callbacks order', setup(async (ctx, h, hf, mount, parent) => {
Expand Down
76 changes: 52 additions & 24 deletions packages/jsx/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,25 @@ type DomApis = Pick<
'document' | 'Node' | 'Text' | 'Element' | 'MutationObserver' | 'HTMLElement' | 'DocumentFragment'
>

/**
* @see https://github.com/preactjs/preact/blob/d16a34e275e31afd6738a9f82b5ba2fb9dbf032b/src/diff/props.js#L107
* @see https://www.measurethat.net/Benchmarks/Show/7818
*/
const propertiesAsAttribute = new Set([
'width',
'height',
'href',
'list',
'form',
/** Default value in browsers is `-1` and an empty string is cast to `0` instead */
'tabIndex',
'download',
'rowSpan',
'colSpan',
'role',
'popover',
])

const isSkipped = (value: unknown): value is boolean | '' | null | undefined =>
typeof value === 'boolean' || value === '' || value == null

Expand Down Expand Up @@ -83,12 +102,21 @@ const walkLinkedList = (ctx: Ctx, el: JSX.Element, list: Atom<LinkedList<LLNode<
)
}

export const reatomJsx = (ctx: Ctx, DOM: DomApis = globalThis.window) => {
const StylesheetId = 'reatom-jsx-styles'
let styles: Rec<string> = {}
let stylesheet: HTMLStyleElement | undefined
export const reatomJsx = (ctx: Ctx, DOM: DomApis = globalThis.window, options?: {
/**
* Adds a style element containing styles from the `css` property to the document.
* @default true
*/
appendStylesheet?: boolean
}) => {
let name = ''

/** @see https://www.measurethat.net/Benchmarks/Show/33272 */
let styles: Rec<string> = {}
let stylesheet: HTMLStyleElement = DOM.document.createElement('style')
stylesheet.id = 'reatom-jsx-styles'
if (options?.appendStylesheet !== false) DOM.document.head.appendChild(stylesheet)

let set = (element: JSX.Element, key: string, val: any) => {
if (key.startsWith('on:')) {
key = key.slice(3)
Expand All @@ -100,13 +128,6 @@ export const reatomJsx = (ctx: Ctx, DOM: DomApis = globalThis.window) => {
if (val == null) element.style.removeProperty(key)
else element.style.setProperty(key, String(val))
} else if (key === 'css') {
stylesheet ??= DOM.document.getElementById(StylesheetId) as any
if (!stylesheet) {
stylesheet = DOM.document.createElement('style')
stylesheet.id = StylesheetId
DOM.document.head.appendChild(stylesheet)
}

let styleId = styles[val]
if (!styleId) {
styleId = styles[val] = `${name ? name + '.' : ''}${random(0, 1e6).toString()}`
Expand All @@ -122,21 +143,28 @@ export const reatomJsx = (ctx: Ctx, DOM: DomApis = globalThis.window) => {
} else if (key.startsWith('prop:')) {
// @ts-expect-error
element[key.slice(5)] = val
} else if (
!propertiesAsAttribute.has(key)
&& element instanceof DOM.HTMLElement
&& (key in element || key === 'class')
) {
if (key === 'class') key = 'className'
// @ts-ignore
element[key] = val == null ? '' : val
} else {
if (key.startsWith('attr:')) {
key = key.slice(5)
}
if (key === 'className') key = 'class'
if (val == null || val === false) element.removeAttribute(key)
else {
val = val === true ? '' : String(val)
/**
* @see https://measurethat.net/Benchmarks/Show/54
* @see https://measurethat.net/Benchmarks/Show/31249
*/
if (key === 'class' && element instanceof HTMLElement) element.className = val
else element.setAttribute(key, val)
}
if (key.startsWith('attr:')) key = key.slice(5)

/**
* @note aria- and data- attributes have no boolean representation.
* A `false` value is different from the attribute not being
* present, so we can't remove it. For non-boolean aria
* attributes we could treat false as a removal, but the
* amount of exceptions would cost too many bytes. On top of
* that other frameworks generally stringify `false`.
*/
if (val == null || (val === false && key[4] !== '-')) element.removeAttribute(key)
else element.setAttribute(key, key == 'popover' && val == true ? '' : val)
}
}

Expand Down