Both className and class attributes created. about hyperapp HOT 39 CLOSED

@jbucaran Good idea.
Haha I'm not against jsx but in the quest of a minimal setup with es6 native solutions ⚔️

from hyperapp.

@selfup I think they're different, but let me investigate further, they might be related after all.

from hyperapp.

@tunnckoCore I can confirm both class and className are added when using hyperx, but definitely not when using JSX.

from hyperapp.

My guess is that they want to be able to put class instead of className in the markup.
It makes sense as unlike jsx, markup is a string, so there is no reserved keyword to handle.
But in the same time they also want to be react-compatible, react only supports className.

from hyperapp.

@jbucaran fyi choojs/hyperx#49

from hyperapp.

PR has been merged and starting from hyperx 2.2.0 you can now disable this:

const { h, app } = require("hyperapp")
const hyperx = require("hyperx")
const html = hyperx(h, { attrToProp: false })

from hyperapp.

I think I have seen something similar here:

http://codepen.io/selfup/pen/WRgOrR

To reproduce

Make two ideas

Hit X on the latest idea (top)

The X button is still highlighted on the idea that was not clicked 🤔

It seems that this happens when a node replaces an old node

Exceptions

If you delete the bottom idea, the top one stays "clean"

from hyperapp.

@selfup It seems we're talking about two different bugs here?

One with className (safely) polluting the element's HTML and another one which is the one that can be seen in @selfup's CodePen.

Both need fixing, but I'm more interested in fixing the second one as that's a bug that has been bothering me in the TodoMVC implementation for a while.

from hyperapp.

We might very well be, but I am wondering if it has to do with the diffing.

If you want I can submit a new issue 😄

Or we can just keep track of it in here

from hyperapp.

@selfup @tunnckoCore Could this be a bug/feature of Hyperx? This does not occur when using JSX.

from hyperapp.

dont think so. try raw h calls?

i'll do a pen and will see if when it appears

from hyperapp.

@jbucaran Both examples with issues are JSX apps.

from hyperapp.

@selfup Alright, can we open a new issue to track the bug you described here?

Let's keep this issue about the class/className bug.

@tunnckoCore Can you confirm this occurs when using JSX too?

from hyperapp.

@tunnckoCore @selfup The reason you have both classname and class is because you're using className in the JSX.

See: https://github.com/selfup/hyperapp-one/blob/master/src/views/counter.js#L10.

@selfup Consider using only class? We're not constrained by this.

from hyperapp.

@jbucaran I have the same initial issue with duplicated classname and class attributes on each element.
I'm using hyperx and the class attribute (not className).
I don't have any actions, just a static model, so there is no diffing involved (afaik).

Do you have a clue on this particular bug?

from hyperapp.

@ngryman Yes, I just had a chance to look at this.

See this Pen.

from hyperapp.

Hyperx adds a classname attribute to the virtual node data object. In Hyperapp we just go ahead and set that property in the element which causes the class to be set correctly.

I don't understand why Hyperx does this, however. This is the commit that introduces the functionality in Hyperx.

• choojs/hyperx#22

from hyperapp.

Ok, I get it.
Any chance this does not work for some reason: https://github.com/feross/hyperscript-attribute-to-property/blob/master/index.js#L14?

from hyperapp.

@ngryman Well observed, this is definitely odd and requires some investigation. 🤔

from hyperapp.

@jbucaran On my local machine, only className is left so class is well deleted.
I can investigate a little on this...

from hyperapp.

@ngryman Wait, it's working, so it's class what gets deleted.

from hyperapp.

Given this view:

<div class="hi">Hello</div>

JSX produces:

{
  children: ["Hello"],
  data: {
    class: "hi"
  },
  tag: "div"
}

Hyperx produces:

{
  children: ["Hello"],
  data: {
    className: "hi"
  },
  tag: "div"
}

from hyperapp.

Using hyperx I have a different result.

Source

const { h } = require('hyperapp')
const hyperx = require('hyperx')
const html = hyperx(h)

const vnode = html`<div class="foo">`
console.log(vnode)

Output

{
  children: [],
  data: {
    className: "foo"
  },
  tag: "div"
}

EDIT: Didn't see you preceding comment 👻

from hyperapp.

It makes sense as hyperscript-attribute-to-property proxy does replace class with className. hyperx probably expects the rendering framework to transform className into a class attribute.

from hyperapp.

@ngryman Hmm, that sounds right. But why?

from hyperapp.

Either you could patch your code to revert className to class or we could file hyperx an issue to ask them if it's possible to add some kind of opt-in for this transform.

from hyperapp.

🤔 That seems like out of the scope of Hyperx.

from hyperapp.

Either you could patch your code to revert className to class or we could file hyperx an issue to ask them if it's possible to add some kind of opt-in for this transform.

Sounds like a plan: choojs/hyperx#48.

from hyperapp.

hallelujah

from hyperapp.

@jbucaran Another issue related to this one is that for svg elements only classname attribute is kept so it's basically impossible to style svg elements for now 😢

I have a feeling the hyperx issue may take some time to resolve, should we implement a temporary fix that just reverts className to class?

from hyperapp.

@ngryman You against JSX?

from hyperapp.

@ngryman I can get that, so then why not fork hyperx, fix the issue (send a PR along upstream too) and use your fork in the meantime.

from hyperapp.

Good job @ngryman! 🍟 🍔

from hyperapp.

@ngryman How would a PR for hyperapp that deals with this look like?

const html = hyperx(h, { attrToProp: false })

it's kinda ugly. Maybe we can take care of it for all the Hyperx-folk using hyperapp :)

from hyperapp.

I totally agree it's ugly 😢

TBH if we think about the full boilerplate, it is kinda heavy too:

import { h } from 'hyperapp'
import hyperx from 'hyperx'

const html = hyperx(h)

We should find a solution to wrap and expose this in some way.

Peer dependency

We could reference hyperx as a peer dependency and then export a helper html:

// html.js

import { h } from 'hyperapp'
import hyperx from 'hyperx'

export default hyperx(h, { attrToProp: false })

So now we can simply do:

import { html } from 'hyperapp'

New package

We could create something like hyperapp-hyperx and do the same. It's not my favorite but it preserves SoC.

@jbucaran If you have other ideas? I don't know how others deal with this.

from hyperapp.

We could reference hyperx as a peer dependency and then export a helper html:

It was like that when I first released the project. The problem is: I couldn't find a way to support both Hyperx and JSX in an unopinionated way.

There may be other template literal or JSX-like libraries in the future and I'd like to support them all if possible.

from hyperapp.

@ngryman

TBH if we think about the full boilerplate, it is kinda heavy too:

import { h } from 'hyperapp'
import hyperx from 'hyperx'
const html = hyperx(h)

There's no way about it unless we make other compromises. I tried something, but was ignored too.

See: https://github.com/substack/hyperxify/pull/8

from hyperapp.

I get it, why not something like installing any template function and then get it via hyperapp?
Naming is subject to change but this is the core idea.

This would be called once to install the template function, in index.js for example:

import { app } from 'hyperapp'
import hyperx from 'hyperx'

app(/*...*/, template: hyperx)

And would be used like this in the views:

import { html } from `hyperapp'

from hyperapp.

@ngryman Thanks, but there's absolutely no way to go around the hyperx boilerplate, and I tried to address that in substack/hyperxify#8.

Even if we export an html module out of the box, the boilerplate will have to be written somewhere, and worse, it will be impossible to use hyperxify to remove the hyperx parser from your application bundle.

The reason for the current boilerplate is not to "punish" hyperx users, on the contrary, it's to make using hyperx with hyperapp not suck (consider the alternative, sending the entire parser down the wire, not to mention it's slow, but the bytes!).

Finally, I think the syntax you've proposed doesn't work with JSX either, so that would certainly punish JSX, which whether we like it or not, are most likely the majority.

from hyperapp.