SVG attributes from JSX unrecognized

[irisjae]

HI Adam again. Last I've mentioned to you, that I'd have to write a small utility to convert svg to jsx, which would then be converted by surplus. So I went and looked, and lo, the React community had already written such a thing. And happily, I went and wrote a little function to translate svg to jsx, then jsx to surplus. However... it doesn't render right! I suspect the reason is because of unrecognized SVG attributes. Let me illustrate.

Let's say I start with this SVG:

<svg viewBox="0 0 100 100">
    <line x1="0" y1="50" x2="100" y2="50" stroke="black" stroke-width="100"></line>
</svg>

It's a pretty idiotic way to make a black square.

Now I run it through JSX:

<svg viewBox="0 0 100 100">
    <line x1={0} y1={50} x2={100} y2={50} stroke="black" strokeWidth={100} />
</svg>

All good. But look again! The stroke-width is now strokeWidth! Turns out (scroll to the bottom) (another link) that SVG attributes are "supported" by JSX. Which seems to mean that the canonical attribute version is camelCased (and thus, basically all svg attributes involving a dash are inflicted). Here is the place where surplus isn't supporting it, and if you run the surplus .compile () (0.5.0-beta11) through this, it becomes:

(function () {
    var __, __line1;
    __ = Surplus.createSvgElement('svg', null, null);
    Surplus.setAttribute(__, "viewBox", "0 0 100 100");
    __line1 = Surplus.createSvgElement('line', null, __);
    Surplus.setAttribute(__line1, "x1", 0);
    Surplus.setAttribute(__line1, "y1", 50);
    Surplus.setAttribute(__line1, "x2", 100);
    Surplus.setAttribute(__line1, "y2", 50);
    Surplus.setAttribute(__line1, "stroke", "black");
    Surplus.setAttribute(__line1, "strokeWidth", 100);
    return __;
})()

Which renders to

<svg viewBox="0 0 100 100">
    <line x1="0" y1="50" x2="100" y2="50" stroke="black" strokeWidth="100"></line>
</svg>

Just a meager line (strokeWidth is not an svg attribute and has no effect).

I'll see if I have time for a PR.

(By the way, turns out the JSX spec seems to say namespaced attributes go with colons... except that the rest of the community and React say they go camelCased.)

[adamhaile]

Ah, yep, we need another translation layer to go from prop to attr names for JSX. I can handle this. I'd like to do a small refactor to move this determination into the transform layer anyway.

Thanks, btw, for giving the SVG support such a thorough runthrough. It's great to suss out these bugs!

Yeah, I saw that part of the JSX spec too. Wish they'd stayed with namespace support. Here's the issue where they debated it and abandoned namespaces: facebook/react#760 (comment) .

[adamhaile]

I just finished a bunch of work to make the field names smarter. The stokeWidth JSX property should now set the stroke-width attribute -- let me know if not!

[adamhaile]

Hmm, I think my recent changes may have broken the viewBox attribute, though -- it might get rendered as view-box. I'll add some special cases.

[adamhaile]

SVG attributes are so weird -- some are snake-cased, some camelCased. I added special cases for the camelCased attributes, so I believe your example should now work. In fact, the original SVG, with stroke-width, should work now too.

[irisjae]

The patch seems to work pretty well for my SVGs :D! Love your work, as usual!

From my pedantic side... Though I'm only using custom attributes only on SVG, (which means that by the current surplus implementation, it works), it should work on HTML too, given that JSX supports it that way (and also because I intend to use it that way). Using this example, both

<div custom-attribute="some-value" />

and

<svg custom-attribute="some-value" />

should compile the custom attribute to an attribute, but compiling them via surplus yields

(function () {
    var __;
    __ = Surplus.createElement('div', null, null);
    __.customAttribute = "some-value";
    return __;
})()

and

(function () {
    var __;
    __ = Surplus.createSvgElement('svg', null, null);
    Surplus.setAttribute(__, "custom-attribute", "some-value");
    return __;
})()

respectively.

Thanks for your excellent work!

[adamhaile]

Not pedantic at all! I agree that a user would generally expect a field named something like custom-attribute to be assigned as an attribute, not a property customAttribute. This should be an easy fix.

[adamhaile]

Custom attributes -- identified by the fact that they contain a dash -- should now always set attributes, not properties.

[irisjae]

I hadn't closed this because this example

<div mycustomattribute="something" />

still doesn't work. I intended to try to make a pull request before bringing this up, but I saw you just brought it from beta to stable! So here it is.

[adamhaile]

Yeah, I was getting increasing reports of people pulling the old 0.4.x branch from npm and hitting all kinds of issues (because it had all kinds of issues). So I felt it was time to jump. It's still an 0.5, not the end of the world :).

I've never quite understood React's philosophy here: it's an attribute-based library that uses DOM property names (well, except for a few where they don't like the DOM capitalization, like spellCheck). If you're going to use property names ... why not set properties? Isn't that even more "Javascript-first"? There may be a good reason, I'm just not aware of the rationale. Maybe compat with old browsers, where the DOM properties weren't as standardized?

So React sets attributes unless it knows a field is better set as a property. Surplus does the opposite, setting properties unless it knows an attribute is preferable. This means that if we know nothing about your field -- it's custom -- it winds up in different places.

React deserves a lot of credit that most users of the library have no idea whether they're setting attributes or properties -- it "just works." I'd like Surplus to be there too.