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

Property-names get lower-cased? #46

Closed
mindplay-dk opened this issue Sep 26, 2019 · 3 comments · Fixed by #47
Closed

Property-names get lower-cased? #46

mindplay-dk opened this issue Sep 26, 2019 · 3 comments · Fixed by #47

Comments

@mindplay-dk
Copy link
Contributor

It looks like property-names are getting lower-cased?

For example, <div className="red"> gets rendered as <div classname="red">, which doesn't work, since the property-names are case-sensitive:

image

I've been looking through the code, and can't figure out where or how this happens.

If I take the same example and import h and render from e.g. preact, it works fine, so I don't think it's CodeSandbox mangling the code or something? 🤔

I only see one call to toLowerCase() in the code, and it applies only to event-handlers.

Any ideas?

@yisar
Copy link
Collaborator

yisar commented Sep 26, 2019

Attribute names are automatically lowercase, which is a requirement of HTML

We should use class instead of className, use onInput instead of onChange

facebook/react#13525

React may also remove className from the React Fire, making it more modern and standard in lowercase and less expensive.

@mindplay-dk
Copy link
Contributor Author

Attribute names are automatically lowercase, which is a requirement of HTML

Yes, for attributes - className isn't an attribute, it's a property.

I brought this up earlier here.

JSX attributes generally map to object properties first - and to HTML attributes only as a fallback. This is true in React and most React-likes.

For example, here you can see Preact prefers object properties:

https://github.com/preactjs/preact/blob/master/src/diff/props.js#L97

As does Superfine:

https://github.com/jorgebucaran/superfine/blob/master/src/index.js#L33

The issue here is type-safety. Attributes do not have types - they're always strings, while the values you work with in JS have other types.

For example, <input type="checkbox" defaultChecked={true}/> is expected to the set the defaultChecked property, which is a boolean, not a string.

This is why JSX attributes map to properties and only fall back to attributes.

We should use class instead of className

When properties and attributes are supported, you can use either. 🙂

use onInput instead of onChange

You don't have to do anything to make that work - in current versions of React, onChange works like the DOM input event. For example, Preact without the compat add-on doesn't do anything specific for either input or change events - it just maps all event names directly to addEventListener() calls, and it just works.

React may also remove className from the React Fire, making it more modern and standard in lowercase and less expensive.

This is similar to what Preact does already:

https://github.com/preactjs/preact/blob/master/src/diff/props.js#L47

It just changes class to className, the property, which may be marginally faster than setting the class attribute, though this is most likely negligible. They're making this change because <div class="foo"> is what people are used to seeing in HTML and it's easier to type.

Anyhow, I understand the problem now: properties aren't supported at all right now, and the browser normalizes className to classname when setAttribute() gets called.

I'll see about adding some tests for this and submit another PR. 👍

@yisar
Copy link
Collaborator

yisar commented Sep 26, 2019

Looking forward to refactoring here, becoming safer and tinyer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants