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

Warning: SVG property className is deprecated #6211

Closed
gaearon opened this Issue Mar 8, 2016 · 14 comments

Comments

Projects
None yet
10 participants
@gaearon
Member

gaearon commented Mar 8, 2016

In #5714, we changed SVG attribute logic to pass all SVG attributes unchanged.

This means, for example, that instead of <svg strokeWidth> you’d write <svg stroke-width>.
The reason for this change was that we don’t want to maintain a whitelist of attributes.

Since we released 15 RC, I saw a report on Twitter saying that <svg className> now also prints a warning:

Warning: SVG property className is deprecated. Use the original attribute name class for SVG tags instead.

In a way, this makes sense, as it is consistent with us not maintaining a whitelist, and with how we treat custom components.

On the other hand, SVG elements do not seem that special, and asking people to use class on them instead invites the old discussion about doing that for DOM elements as well.

I would like to double-check that this className => class is indeed a change we want to make for SVG elements. Alternatively, we can leave className as is by introducing a special case for it alone.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Mar 8, 2016

Contributor

Yes, this was intentional. We could introduce a special case for className, but then the behavior would be different from the webcomponent behavior, so we're going to have divergent behavior either way. This change was intentional, the simple rule "we pass svg attributes through" works well, and we've already released the RC, so let's close this out.

As per our discussion on the team chat, we can provide a codemod to facilitate an easy migration: #6213

Contributor

jimfb commented Mar 8, 2016

Yes, this was intentional. We could introduce a special case for className, but then the behavior would be different from the webcomponent behavior, so we're going to have divergent behavior either way. This change was intentional, the simple rule "we pass svg attributes through" works well, and we've already released the RC, so let's close this out.

As per our discussion on the team chat, we can provide a codemod to facilitate an easy migration: #6213

@jimfb jimfb closed this Mar 8, 2016

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 8, 2016

Member

I don't remember the rationale here. We don't use class for HTML elements so why should we on SVG when there is a perfectly fine property name?

I interpreted the change as "we don't block other attributes" but we can still make special cases just like we do for HTML.

Web Components is only different because there is no other way and support is busted anyway. Discrepancy between HTML/SVG which have merged into a single integrated standard and are both core features of the platform seems worse than discrepancy between the core features and a feature that nobody really uses.

I don't really like that we have this discrepancy without a clearer story around it (and without a major section describing in the blog post).

Member

sebmarkbage commented Mar 8, 2016

I don't remember the rationale here. We don't use class for HTML elements so why should we on SVG when there is a perfectly fine property name?

I interpreted the change as "we don't block other attributes" but we can still make special cases just like we do for HTML.

Web Components is only different because there is no other way and support is busted anyway. Discrepancy between HTML/SVG which have merged into a single integrated standard and are both core features of the platform seems worse than discrepancy between the core features and a feature that nobody really uses.

I don't really like that we have this discrepancy without a clearer story around it (and without a major section describing in the blog post).

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Mar 8, 2016

Contributor

... but we can still make special cases just like we do for HTML.

Special cases suck. You have differences anyway because React html elements use camelCase and svg attributes use kebab-case, among other things... unless you want to write some fancy logic to try and normalize the differences and automatically apply transforms to recapitalize and insert hyphens. But it's pointless to perform computation and special-case checks when you could just pass them through. It's much cleaner to have a simple "pass-through" rule for svg, and we should apply that rule consistently.

EDIT: Actually, you couldn't write an automatic rule, because SVG is inconsistent about the names of their attributes. You have camelCase attributes like pointsAtY and surfaceScale, but you also have kebab-case ones like underline-position and unicode-range. You're not going to be able to get around the inconsistencies between SVG and HTML.

Contributor

jimfb commented Mar 8, 2016

... but we can still make special cases just like we do for HTML.

Special cases suck. You have differences anyway because React html elements use camelCase and svg attributes use kebab-case, among other things... unless you want to write some fancy logic to try and normalize the differences and automatically apply transforms to recapitalize and insert hyphens. But it's pointless to perform computation and special-case checks when you could just pass them through. It's much cleaner to have a simple "pass-through" rule for svg, and we should apply that rule consistently.

EDIT: Actually, you couldn't write an automatic rule, because SVG is inconsistent about the names of their attributes. You have camelCase attributes like pointsAtY and surfaceScale, but you also have kebab-case ones like underline-position and unicode-range. You're not going to be able to get around the inconsistencies between SVG and HTML.

@albertfdp

This comment has been minimized.

Show comment
Hide comment
@albertfdp

albertfdp Mar 8, 2016

Being className the property name for HTML elements, it is difficult to tell a story around why SVGs should use class instead, especially for beginners. In my humble opinion, even though this might affect Web Components, it makes things easier if there is internal consistency on how to use this property.

Regarding special cases, they suck yes, but it sucks even more to use different naming for the same property if HTML element or SVG.

albertfdp commented Mar 8, 2016

Being className the property name for HTML elements, it is difficult to tell a story around why SVGs should use class instead, especially for beginners. In my humble opinion, even though this might affect Web Components, it makes things easier if there is internal consistency on how to use this property.

Regarding special cases, they suck yes, but it sucks even more to use different naming for the same property if HTML element or SVG.

@jimfb

This comment has been minimized.

Show comment
Hide comment
@jimfb

jimfb Mar 8, 2016

Contributor

Looks like there is more discussion here.

Contributor

jimfb commented Mar 8, 2016

Looks like there is more discussion here.

@jimfb jimfb reopened this Mar 8, 2016

@adidahiya

This comment has been minimized.

Show comment
Hide comment
@adidahiya

adidahiya Mar 8, 2016

This attribute name change also caught me by surprise. It makes it difficult to write components that work in both HTML and SVG contexts. I have a tooltip component like this that mostly* worked before v15.

* save for some IE-related SVG bugs that were present before v15

adidahiya commented Mar 8, 2016

This attribute name change also caught me by surprise. It makes it difficult to write components that work in both HTML and SVG contexts. I have a tooltip component like this that mostly* worked before v15.

* save for some IE-related SVG bugs that were present before v15

@koba04

This comment has been minimized.

Show comment
Hide comment
@koba04

koba04 Mar 9, 2016

Contributor

Do you have any plans for using class instead of className in HTML elements?
I guess it sounds good.

It seems React can use class as the property name of ReactElement because React no longer actively support IE8.

Are there any reasons using className instead?

Contributor

koba04 commented Mar 9, 2016

Do you have any plans for using class instead of className in HTML elements?
I guess it sounds good.

It seems React can use class as the property name of ReactElement because React no longer actively support IE8.

Are there any reasons using className instead?

@Swatinem

This comment has been minimized.

Show comment
Hide comment
@Swatinem

Swatinem Mar 9, 2016

I use a CSS-in-JS solution with a helper function that splats in {className, style} to mix and match pre-defined styles with true inline styles. I use it for both html (obviously) and svg (to override fill, height, etc).

It is kind of annoying to have this warning now. So whatever you decide to do, at least make it consistent across html and svg. I personally would prefer to remove as much whitelisting / special casing as possible.

Swatinem commented Mar 9, 2016

I use a CSS-in-JS solution with a helper function that splats in {className, style} to mix and match pre-defined styles with true inline styles. I use it for both html (obviously) and svg (to override fill, height, etc).

It is kind of annoying to have this warning now. So whatever you decide to do, at least make it consistent across html and svg. I personally would prefer to remove as much whitelisting / special casing as possible.

@adidahiya

This comment has been minimized.

Show comment
Hide comment
@adidahiya

adidahiya Mar 9, 2016

Renaming classNameclass for HTML elements seems fine to do in the long run as @koba04 suggests, but that sounds like a much bigger breaking change. I'd prefer if React kept the prop name consistent across all kinds of elements. Any chance this might be reverted before official v15.0.0?

adidahiya commented Mar 9, 2016

Renaming classNameclass for HTML elements seems fine to do in the long run as @koba04 suggests, but that sounds like a much bigger breaking change. I'd prefer if React kept the prop name consistent across all kinds of elements. Any chance this might be reverted before official v15.0.0?

@koistya

This comment has been minimized.

Show comment
Hide comment
@koistya

koistya Mar 9, 2016

Contributor

A warning message is in place, no special case for the "class" attribute is
good. What you can do though, is to include a link to the blog post
explaining the reasoning behind it into the warning message. Also, I have a
filling that you will want to change "className" to "class" for HTML elements
in the near future, that would be awesome :)

Contributor

koistya commented Mar 9, 2016

A warning message is in place, no special case for the "class" attribute is
good. What you can do though, is to include a link to the blog post
explaining the reasoning behind it into the warning message. Also, I have a
filling that you will want to change "className" to "class" for HTML elements
in the near future, that would be awesome :)

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Mar 9, 2016

Member

Any chance this might be reverted before official v15.0.0?

We're exploring a couple options. I think at the very least we'll make sure className continues to work. The extreme option might be to backout all of this SVG work and get us back to 0.14 support. That's obviously not ideal but is available to us. I think I'm personally leaning towards backpedaling on the "pass everything through" approach and expand the list of supported attributes in the whitelist. This would mean we'd no longer support hyphenated attributes and would go back to the camelCase mapping. I don't see this as a huge problem as it maintains current (0.14) behavior, matches what we do for HTML attrs, and is something we have to continue doing for a handful of attributes anyway for the namespaced attrs (xlink:href).

Renaming classNameclass for HTML elements seems fine to do in the long run ... but that sounds like a much bigger breaking change

Maybe we could do it but it definitely is a very big change. We might be too far down this path and honestly by the time we feel ready for it, we might all be using abstractions on top of the DOM (something like react-native-web for example), so it might not matter as much. I agree with what I think you're getting at though - we should probably do it all at once and not live in this hybrid world where SVG uses one pattern and HTML uses another.

Member

zpao commented Mar 9, 2016

Any chance this might be reverted before official v15.0.0?

We're exploring a couple options. I think at the very least we'll make sure className continues to work. The extreme option might be to backout all of this SVG work and get us back to 0.14 support. That's obviously not ideal but is available to us. I think I'm personally leaning towards backpedaling on the "pass everything through" approach and expand the list of supported attributes in the whitelist. This would mean we'd no longer support hyphenated attributes and would go back to the camelCase mapping. I don't see this as a huge problem as it maintains current (0.14) behavior, matches what we do for HTML attrs, and is something we have to continue doing for a handful of attributes anyway for the namespaced attrs (xlink:href).

Renaming classNameclass for HTML elements seems fine to do in the long run ... but that sounds like a much bigger breaking change

Maybe we could do it but it definitely is a very big change. We might be too far down this path and honestly by the time we feel ready for it, we might all be using abstractions on top of the DOM (something like react-native-web for example), so it might not matter as much. I agree with what I think you're getting at though - we should probably do it all at once and not live in this hybrid world where SVG uses one pattern and HTML uses another.

@JKillian

This comment has been minimized.

Show comment
Hide comment
@JKillian

JKillian Mar 10, 2016

I think I'm personally leaning towards backpedaling on the "pass everything through" approach and expand the list of supported attributes in the whitelist.

Would the list be fairly complete? I know in previous iterations of React it was missing quite a large number of tags/attributes. It's tough as just one or two of these missing can make things a lot more painful in your application's code. That being said, definitely agree with this:

not live in this hybrid world where SVG uses one pattern and HTML uses another.

JKillian commented Mar 10, 2016

I think I'm personally leaning towards backpedaling on the "pass everything through" approach and expand the list of supported attributes in the whitelist.

Would the list be fairly complete? I know in previous iterations of React it was missing quite a large number of tags/attributes. It's tough as just one or two of these missing can make things a lot more painful in your application's code. That being said, definitely agree with this:

not live in this hybrid world where SVG uses one pattern and HTML uses another.

@zpao

This comment has been minimized.

Show comment
Hide comment
@zpao

zpao Mar 10, 2016

Member

Yea I think it would have to be totally complete.

On Wednesday, March 9, 2016, Jason Killian notifications@github.com wrote:

I think I'm personally leaning towards backpedaling on the "pass
everything through" approach and expand the list of supported attributes in
the whitelist.

Would the list be fairly complete? I know in previous iterations of React
it was missing quite a large number of tags/attributes. It's tough as just
one or two of these missing can make things a lot more painful in your
application's code. That being said, definitely agree with this:

not live in this hybrid world where SVG uses one pattern and HTML uses
another.


Reply to this email directly or view it on GitHub
#6211 (comment).

Member

zpao commented Mar 10, 2016

Yea I think it would have to be totally complete.

On Wednesday, March 9, 2016, Jason Killian notifications@github.com wrote:

I think I'm personally leaning towards backpedaling on the "pass
everything through" approach and expand the list of supported attributes in
the whitelist.

Would the list be fairly complete? I know in previous iterations of React
it was missing quite a large number of tags/attributes. It's tough as just
one or two of these missing can make things a lot more painful in your
application's code. That being said, definitely agree with this:

not live in this hybrid world where SVG uses one pattern and HTML uses
another.


Reply to this email directly or view it on GitHub
#6211 (comment).

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Mar 15, 2016

Member

Closing as #5714 was reverted in #6243.

Member

gaearon commented Mar 15, 2016

Closing as #5714 was reverted in #6243.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment