JSX tag with className attribute render a dom node with both class and className attribute

[leifoolsen]

The following example illustrates the issue:

const Button = ({label}) =>
  <button className="my-button">{label}</button>
;

const view = () =>
  <div>
    <Button label="Click me" />
  </div>
;

The rendered output (in Chrome, macOS High Sierra)

<button class="my-button" classname="my-button">Click me</button>

If I use class instead of className it renders correctly.

const Button = ({label}) =>
  <button class="my-button">{label}</button>
;

[jorgebucaran]

@leifoolsen We'll definitely fix this!

See also: https://github.com/picodom/picodom/pull/70

[jorgebucaran]

@leifoolsen The solution proposed above creates more problems and I decided to remove it from core. The suggestion I can give you is to not use className. There is no reason to use className since class works just as well and we do not intend to be compatible with React.

[leifoolsen]

I agree. Using "class" is better.

[mindplay-dk]

Any idea why/how this broke SVG? Can we fix it?

class is not the only thing affected - for example, form elements get unnecessary value and checked attributes etc... Duplicating all property/attribute updates also sounds like unnecessary overhead - it's a bit of a hack.

Other engines manage to do property updates without duplicating everything, and all support SVG, so there must be an answer.

[jorgebucaran]

@mindplay-dk Sure, it's fixable, but it's a lot of code, and I don't consider this to be an important issue.

[mindplay-dk]

The difference was 20 bytes, as I recall? I don't think that's "a lot of code".

It seems haphazard to just litter DOM objects both with useless properties and meaningless attributes - I wish you wouldn't prioritize byte-savings over getting this right, but I don't know what else to say to make you change your mind.

To me, it's critical that the core component managing all the DOM updates of my UI code is making only exactly the expected, required, correct DOM changes. It's critical that the code is simple, readable, easy to understand, and fairly small.

Given that most networks can't even ship packets smaller than ~1.6 KB, it's much less critical to me whether or not we stay under some magical 1 KB limit - and especially when that constraint imposes any sort of trade-off against any of the critical priorities.

I'm very much in favor of artistic limitations when those inspire to making a better product - but I'm very much opposed to such limitations becoming dogma and inspiring to any qualitative trade-offs.

[jorgebucaran]

@mindplay-dk I am happy to reconsider, but your solution didn't take into account SVG, and yes, properly written, it was a lot of code, so I decided to keep the current, simpler approach.

You keep talking about "littering the DOM", but have provided little evidence to show this is actually a problem here; className and forms with value or checked attributes is a petty issue.

I would prefer if, instead of philosophizing, we could address what are the critical limitations of the current approach. That would lead to understanding, and understanding would lead to making a better-informed decision.

[mindplay-dk]

You keep talking about "littering the DOM", but have provided little evidence to show this is actually a problem here

For one, it can lead to confusion during development - while debugging, you see a particular attribute or object property, and you think it means something, but it's just garbage we produce because it was easier to implement it that way, not because it makes sense.

Reading the code the first time is also confusing.

I'd rather not waste time procuring evidence that the duplicate initialization (and string conversions etc.) of every property/attribute causes performance or memory overhead - that really seems like a waste of time.

I'd rather spend my time bringing us closer to doing the correct updates.

[jorgebucaran]

@mindplay-dk No worries. You don't need to collect the evidence (e.g., what garbage? what confusion?), but I'll definitely need "some" to justify adding a bunch of code and complicating the updateElementData algorithm for a tiny non-issue.

[mindplay-dk]

In a nutshell, for me, the issue is that, with every expected property/attribute update, we're also doing an unexpected property/attribute update.

For me, this is about expectations, in the "do what you say" sense - in terms of documentation, developer expectations, an so on, no way is anybody expecting a meaningless attribute every time they're trying to set a property, nor vice-versa.

In terms of documentation, you'd be hard pressed to come up with a meaningful explanation for why we do this, since there is no meaningful use-case for this as a "feature" - the best you can do is pretend it doesn't exist, and hope that nobody will care or notice.

That also means, lets hope developers don't actually start counting on this as a "feature", if one day we figure out a simple way to fix it, which would then be a "breaking change" - for example, designers might start targeting CSS attributes/values just because they're there and it's faster than asking a developer to add something to a component or model, and developers might start to expect that attribute-values will be available to read directly from the DOM object.

You could label that as a "feature", but I think we both know you didn't design it that way 😉

[jorgebucaran]

@mindplay-dk Attributes and props are just weird. With Hyperapp it just works, no need to worry about it.

TBH I've lost the thread of the conversation (again). I really don't know what we are talking about anymore. I don't mean that in a cynical way! I am just lost in all the text we are throwing at each other. 😓

To summarize: I don't care as long as it works. If something is broken, e.g., you are unable to do something or a strange bug is causing your app to crash, then I would be interested in fixing it.

[mindplay-dk]

I can't predict the kind of bugs that might emerge from random properties and attributes arising everywhere as a side-effect of the implementation.

I guess we'll wait and find out 😆

[jorgebucaran]

Random attributes? What do you mean?

If you are using as attributes anything other than oncreate, onupdate, onremove, ondestroy, keys, and any DOM attributes, please don't. That's not supported.

[mindplay-dk]

I meant "unexpected", not "random", sorry.

[jorgebucaran]

What would those be and how could they affect the app is what I would like to find out.

Can we shift the convo to that? Else, I don't see how we move forward from here.

[mindplay-dk]

I can't predict the kind of bugs that might emerge from random unexpected properties and attributes arising everywhere as a side-effect of the implementation

I can't convince you that's a problem, so lets just move on 👍

[jorgebucaran]

That's only the how part no? What about the what part?

I think the problem is that we have two clashing approaches to solving this issue. Your approach is definitely lawful good, while mine is definitely chaotic, arguably good or neutral.

[SkaterDad]

@JorgeBucaran while mine is definitely chaotic, arguably good or neutral

I'd call your approach pragmatic.

[mindplay-dk]

Well, I’ve decided I can live with this for now - if this thing really catches on and the SAM pattern works out the way I’m hoping for, I might take another stab at this, but for now, it’s functional.

Good enough for government work 😂

[jorgebucaran]

@mindplay-dk Okay, I fixed this.

• https://github.com/hyperapp/hyperapp/commit/4f147c2ec00fd2a69b2298981a81372c603211ca

I found at least another property that was problematic, innerHTML. I don't know if this was pointed out to me before, but it came up in a discussion with @frenzzy. 🎉