svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Custom element boolean attribute incorrectly converted to `true`

Open CLDXiang opened this issue 1 year ago • 7 comments

Describe the bug

When passing a boolean attribute to a custom element, the prop value is alway true.

Reproduction

https://github.com/CLDXiang/svelte-custom-elements-attribute-bug

pnpm install
pnpm dev

In demo/App.svelte:

<my-component a-boolean={false} />

But the rendered aBoolean value is true.

Logs

No response

System Info

System:
    OS: macOS 13.6.1
    CPU: (20) x64 Intel(R) Core(TM) i9-10910 CPU @ 3.60GHz
    Memory: 133.66 MB / 32.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 16.20.1 - ~/.nvm/versions/node/v16.20.1/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.20.1/bin/yarn
    npm: 8.19.4 - ~/.nvm/versions/node/v16.20.1/bin/npm
  Browsers:
    Chrome: 123.0.6312.59
    Firefox: 100.0.2
    Safari: 16.6
  npmPackages:
    svelte: ^4.2.12 => 4.2.12

Severity

annoyance

CLDXiang avatar Mar 22 '24 08:03 CLDXiang

The problem is the code generates for

<div class="page">
  <my-component a-boolean={x} />
</div>

What it should be doing:

			div = element("div");
			my_component = element("my-component");
			set_custom_element_data(my_component, "a-boolean", x);
			attr(div, "class", "page");

What is actually does:

			div = element("div");
			div.innerHTML = `<my-component a-boolean="${false}"></my-component>`;
			attr(div, "class", "page");

(you can see this in the js output tab of the REPL)

The fix therefore involves bailing out of the "can use innerHTML here" optimization.

The workaround is to put false into a variable, like so:

<script>
  let x = false;
</script>
<div class="page">
  <my-component a-boolean={x} />
</div>

...but that doesn't fix the issue either, because there's a second bug: set_custom_element data is checking if the given property is on the node, but it does not take into account the attribute alias: It's checking "is a-boolean on the element", but there's only aBoolean, so it fails. I'm not sure what the best fix here is, maybe something like __alias_map on the element which Svelte checks and then tries again with the dom alias. It could also/additionally more generally try to transform foo-bar into fooBar (common pattern) and apply it then.

This bug is present in both Svelte 4 and 5.

dummdidumm avatar Mar 22 '24 09:03 dummdidumm

The workaround is to put false into a variable

image

Thanks for your replying. I tried this workaround, but the aBoolean in MyComponent is still true, and in the HTML it was still rendered as <my-component a-boolean="" ...>. As a newcomer to svelte, I can't fully grasp the second bug mentioned. Dose this second bug relates to the ineffectiveness of the workaround? Or is there another underlying cause that might be contributing to the problem?

CLDXiang avatar Mar 22 '24 12:03 CLDXiang

I'm sorry, I expressed myself poorly there - the workaround would only work if the second bug wasn't there. So: the second bug is a blocker, which needs fixing either way, and once that's done, you could be able to use the workaround. I adjusted the wording in my comment a bit.

dummdidumm avatar Mar 22 '24 14:03 dummdidumm

Checking this again - turns out we do make a conversion inside the attributeChangedCallback hook, but since the boolean false becomes the string false by that time, the logic "should this be converted to true or false" says "only empty strings should be converted to false" (which is correct). I'm now almost tempted to reopen the PR you did initially which treated false as a special case - though it's wrong strictly speaking, the spec says boolean attributes are true if they are anything else than the empty string. There's no good answere here ...

dummdidumm avatar Apr 09 '24 11:04 dummdidumm

I have updated my repro repository with additional usage site demos using Vue and React to provide further context. It appears that when rendering custom elements, all three frameworks exhibit similar behavior regarding the handling of boolean attribute values. Specifically, they convert boolean values to the strings "true" and "false" when setting these attributes, which may not align with the spec 🤔.

Svelte: Pasted Graphic 2

Vue: Pasted Graphic 3

React: Pasted Graphic 4

I am uncertain whether this behavior will change in React 19.

CLDXiang avatar Apr 09 '24 12:04 CLDXiang

@dummdidumm does the core team have any updates to a possible workaround or fix? I'm currently attempting to upgrade my custom elements library from Svelte 3 to Svelte 4 and running into the same exact problem.

tytusplanck avatar Aug 01 '24 13:08 tytusplanck

One workaround is to use an action which is a) only used on the client (no risk of server output being clutter) b) you can set it as a property however you want in there. You could create a generic "set as property" action which takes the property name and the property value to set it accordingly.

In Svelte 5 the "set as property" case is fixed, it will not set it as an attribute (and #12624 could help further to specificy exactly how you want to set it). The problem of dash case -> camel case persists though.

dummdidumm avatar Aug 30 '24 13:08 dummdidumm