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

False arguments serialized as strings #129

Open
dy opened this issue Oct 9, 2019 · 8 comments
Open

False arguments serialized as strings #129

dy opened this issue Oct 9, 2019 · 8 comments
Labels

Comments

@dy
Copy link

dy commented Oct 9, 2019

Suppose use-case for conditional applying classes:

import { render } from "preact";
import { html } from "htm/preact";

let el = document.createElement("div");
render(
  html`
    <div class="${false} ${null} ${undefined} ${"foo"}" />
  `,
  el
);

console.log(el.innerHTML);
// <div class="false null undefined foo"></div> 

Sandbox

That's unexpected that the htm serializes false/null class values literally.
That can be worked around with clsx or alike, but.

@Kanaye
Copy link

Kanaye commented Oct 9, 2019

Your example doesn't show your described behaviour.
Also these values would anyway get stringified when preact applies them to the dom.
You are passing a string with some values so and htm passes you that string.
If you want to pass multiple values as one prop to a component you 'll need to use an array.
Here an example showing htm passing the values "as is" https://codesandbox.io/s/1zzxs

In jsx your example of applying classes wouldn't work that way.
You'd write that somewhat like that:

<div class={[false, null, undefined, "foo"].filter(Boolean).join(' ')}></div>

Which is basicly what clsx is doing.

@dy
Copy link
Author

dy commented Oct 9, 2019

Thank you for the answer, but that question is rather design concern, not looking for workaround. That would be natural to visually declare classes instead of array manipulations, I wonder what are the maintainers' opinion on that - if that's worth considering that case.

@Kanaye
Copy link

Kanaye commented Oct 9, 2019

Okay, what would you consider the expected behaviour ? htm can 't pass it as an array, because you are not passing an array. Also if it would pass an array the rendering framework (e.g. preact) would need to stringify the values, with a slightly different but also not working result.
Also it can't filter out null/undefined values, because Components could expect them.

@dy
Copy link
Author

dy commented Oct 9, 2019

The expected result is the same as when you directly render these values:

html`<div>${ false } ${ null } ${ undefined }</div>`

It renders blank string here.
But in case of class/attributes it does not.

I'd make rather strong guess that null-ish values are meaningless for all string arguments, ie. if anything is wrapped into a string, it must not be serialized literally:

html`<div attr="${false}"/>` // <div attr="" />, not <div attr="false" />
html`<div attr=${false} />` // <div />, not <div attr="false" />

Mb that's a bit too smart though, but practically useful.

@Kanaye
Copy link

Kanaye commented Oct 9, 2019

That is behaviour of preact not htm.
Also in this case htm is only passing the raw values to the bound h function.
Proof: https://codesandbox.io/s/bold-dawn-685mb

Also as I wrote above you can't just remove values as they may be passed to Components using these values and expecting false, undefined or null as a value.
So at least in my opinion it's expected to pass down the raw values and not modify them.

@dy
Copy link
Author

dy commented Oct 9, 2019

you can't just remove values

The problem is not false/null values, but string attributes - htm does rendering part here, meaningless for classes and possibly all string attributes in general:

html`<div attr="foo ${false}"/>`
// expected <div attr="foo" />
// actual <div attr="foo false" />

@developit
Copy link
Owner

developit commented Oct 16, 2019

This is not addressable, since the behaviour described would be specific to a given attribute. For example, htm explicitly cannot treat falsy (combined or otherwise) values within some attributes as empty:

html`<a aria-hidden="${false}">visible</a>`
// should render <a aria-hidden="false">visible</a>

html`<a aria-hidden=" ${false} ">visible</a>`
// should render <a aria-hidden=" false ">visible</a>

While I can see the convenience of special-casing DOMTokenList values in HTM, it's not feasible to implement a scalable solution for doing so.

// classnames
const cx = (...a) => a.filter(Boolean).join(' ');

html`<a class=${cx(false, 'hello', null, undefined)}>visible</a>`
// renders <a class="hello">visible</a>

In terms of my own preference or recommendation, it seems like this would be nicer to address at the pragma layer. Personally I'd just co-opt classList for the purpose:

function h(tag, props, children) {
  if (props && props.classList) props.class = props.classList.filter(Boolean).join(' ');
  return { tag, props, children };
}
const html = htm.bind(h);
html`<a classList=${[false, 'hello', null, undefined]}>visible</a>`
// renders <a class="hello">visible</a>

@jeremy-coleman
Copy link

Just a sidenote, The vscode lit html plugin will warn about this, pretty cool.

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

No branches or pull requests

4 participants