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
Both className and class attributes created. #85
Comments
I think I have seen something similar here: http://codepen.io/selfup/pen/WRgOrR To reproduceMake 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 ExceptionsIf you delete the bottom idea, the top one stays "clean" |
@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. |
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 |
@selfup I think they're different, but let me investigate further, they might be related after all. |
@selfup @tunnckoCore Could this be a bug/feature of Hyperx? This does not occur when using JSX. |
dont think so. try raw h calls? i'll do a pen and will see if when it appears |
@tunnckoCore I can confirm both class and className are added when using hyperx, but definitely not when using JSX. |
@jbucaran Both examples with issues are JSX apps. |
@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? |
@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. |
@jbucaran I have the same initial issue with duplicated Do you have a clue on this particular bug? |
Hyperx adds a I don't understand why Hyperx does this, however. This is the commit that introduces the functionality in Hyperx. |
Ok, I get it. |
@ngryman Well observed, this is definitely odd and requires some investigation. 🤔 |
@jbucaran On my local machine, only |
@ngryman Wait, it's working, so it's |
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"
} |
Using 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 👻 |
It makes sense as |
@ngryman Hmm, that sounds right. But why? |
My guess is that they want to be able to put |
Either you could patch your code to revert |
🤔 That seems like out of the scope of Hyperx. |
Sounds like a plan: choojs/hyperx#48. |
hallelujah |
@jbucaran Another issue related to this one is that for I have a feeling the |
@ngryman You against JSX? |
@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. |
@jbucaran Good idea. |
PR has been merged and starting from const { h, app } = require("hyperapp")
const hyperx = require("hyperx")
const html = hyperx(h, { attrToProp: false }) |
Good job @ngryman! 🍟 🍔 |
@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 :) |
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 dependencyWe could reference // 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 packageWe could create something like @jbucaran If you have other ideas? I don't know how others deal with this. |
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. |
There's no way about it unless we make other compromises. I tried something, but was ignored too. |
I get it, why not something like installing any template function and then get it via This would be called once to install the template function, in import { app } from 'hyperapp'
import hyperx from 'hyperx'
app(/*...*/, template: hyperx) And would be used like this in the views: import { html } 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 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. |
There is
classname
andclass
attributes at the created DOM nodes.Kinda feel that i faced that while decoupling the dom diffing and fixed it but cant remember how and where it was and where the codes are. (just did kinda dozen rewrites of few things in a couple of folders; wrote 3-4 new packages that i am lazy to write tests and docs, haha. i'm totally 🤒 👿 💢)
The text was updated successfully, but these errors were encountered: