Skip to content
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

Passing an object with a custom toString() as a 'class' attribute #401

Closed
BenoitZugmeyer opened this issue Nov 14, 2016 · 14 comments
Closed

Comments

@BenoitZugmeyer
Copy link

I am writing a library using objects with a custom toString() method to render class names. Simplified example:

const object = {
  toString() {
    return computeClassNames();
  },
  //  ... other data
};

element.className = object;

It works great on plain DOM and React, but can't work with Preact because if a class attribute is an object, it will use the object enumerable keys associated with a truthy value to format the classname.

I understand why you chose to do this, and I find it quite useful. But would you consider changing the condition to format the class objects to something like:

if (lastSimple && lastSimple.toString === Object.prototype.toString) {
    attributes.class = hashToClassName(lastSimple);
}

or (might be more robust)

if (lastSimple && String(lastSimple) === '[object Object]') {
    attributes.class = hashToClassName(lastSimple);
}

or (not the cleanest but might be more efficient)

if (lastSimple && Object.prototype.toString.call(lastSimple) === '[object Object]') {
    attributes.class = hashToClassName(lastSimple);
}

I could change my library to return an object with keys as class names, but I'd prefer not to add code specific to preact.

Thank you

@developit
Copy link
Member

@BenoitZugmeyer I'm actually toying with the idea of dropping support for Object class values. If I did, would this method begin to work? It would simply be removing these three lines:

https://github.com/developit/preact/blob/master/src/dom/index.js#L27-L29

@BenoitZugmeyer
Copy link
Author

Just tried commenting those lines, and yes it works as I expect. (Sorry my fix suggestions were on an older version of preact)

@developit
Copy link
Member

heh yeah things are moving quickly around here 😅
I'll add this to my list of nails in the coffin of built-in object class support. Removing it saves some bytes (~150) and speeds things up a tad.

@amccloud
Copy link

amccloud commented Dec 2, 2016

@developit the change @BenoitZugmeyer is proposing should also allow glamor to be used with preact. https://github.com/threepointone/glamor/blob/master/src/index.js#L235-L237

@developit
Copy link
Member

Oooh that's huge, I am actually looking into glamor right now haha. We might need to have a 7.1 and then immediately an 8.x release though, dropping object class prop support is definitely a breaking change.

@developit developit added this to the 8.0 milestone Dec 2, 2016
@wyze
Copy link
Contributor

wyze commented Dec 7, 2016

Came here looking for this since I am using glamor on my personal site. If this change lands, it would be great to get this over on preact-render-to-string also to support SSR.

@developit
Copy link
Member

Absolutely, should be seeing this land very soon. Cheers!

@BenoitZugmeyer
Copy link
Author

So, I tested Preact 8 in a small project of mine, and I found a single issue, related to this ticket. The class attribute is ignored if its value is an object and the element is SVG.

Test case:

  <svg class={{ toString() { return "pwet" } }} />

This is because of this test. I tried to search the history the reason of this condition, it has been introduced in 8ff9e32 without any explaination. Would it be possible to remove the typeof value!=='object' && part?

@NekR
Copy link
Collaborator

NekR commented Apr 6, 2017

I'm not even sure why function test is there. Just do toString() (variable + '') to everything and pass to attribute (unless void/null/false of course).

@BenoitZugmeyer
Copy link
Author

Yes the function test is probably useless too. No need to convert the value to string, setAttribute is doing it anyway (see document.body.setAttribute("bleh", { toString() { return "bleh" } }))

@NekR
Copy link
Collaborator

NekR commented Apr 6, 2017

setAttribute is doing it anyway

I guess some IE or older Android or whatever webkit may reject such direct objects passing and that's why there is a check. Assuming browser does everything right could be a wrong path :-)

@developit
Copy link
Member

We can definitely remove the object check, that'll save bytes and it's actually totally superficial - it was added to avoid setting <div foo="[object Object]">, but doing so isn't really an issue.

The function check is to prevent this, so we'll likely want to keep it:

screen shot 2017-04-06 at 4 16 57 pm

@NekR
Copy link
Collaborator

NekR commented Apr 6, 2017

@developit makes sense :-)

@NekR
Copy link
Collaborator

NekR commented Apr 6, 2017

Here we go: #626

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

No branches or pull requests

5 participants