fix(jsx): props are set as properties #999
Conversation
|
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
packages/jsx/src/index.ts
Outdated
| let name = '' | ||
|
|
||
| DOM.document.head.appendChild(stylesheet) |
There was a problem hiding this comment.
If a user will not using css prop there will unnecessary style element in their DOM.
There was a problem hiding this comment.
I made this change because dynamically adding a style tag can introduce style order. When changing the order, random bugs with styles can occur.
There was a problem hiding this comment.
Agree. Please, add an option to the fabric to get an ability to disable the element insertion.
There was a problem hiding this comment.
Replaced Record with Map, because Map is more productive when dynamically adding and reading.
| @@ -445,38 +439,6 @@ 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) => { | |||
There was a problem hiding this comment.
We need to stay a special case for class and className, it should work without prefixes in both ways.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
kasperskei
force-pushed
the
998-props-as-properties
branch
from
427b844 to
ab42221
Compare
kasperskei
force-pushed
the
998-props-as-properties
branch
from
ab42221 to
68b2f1d
Compare
kasperskei
force-pushed
the
998-props-as-properties
branch
from
68b2f1d to
461c346
Compare
#998