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

Custom element boolean attribute incorrectly converted to true #10871

Open
CLDXiang opened this issue Mar 22, 2024 · 7 comments
Open

Custom element boolean attribute incorrectly converted to true #10871

CLDXiang opened this issue Mar 22, 2024 · 7 comments

Comments

@CLDXiang
Copy link

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

@dummdidumm
Copy link
Member

dummdidumm commented Mar 22, 2024

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.

@CLDXiang
Copy link
Author

CLDXiang commented Mar 22, 2024

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?

@dummdidumm
Copy link
Member

dummdidumm commented Mar 22, 2024

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
Copy link
Member

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 ...

@CLDXiang
Copy link
Author

CLDXiang commented Apr 9, 2024

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.

@tytusplanck
Copy link

@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.

@dummdidumm
Copy link
Member

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.

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

3 participants