Skip to content

Commit

Permalink
fix: correctly set custom element props (#14508)
Browse files Browse the repository at this point in the history
fixes #14391

In #13337 the "when to set as a property" logic for custom elements was adjusted. A bug was introduced during this, and it consists of several parts, of which the latter I'm not sure what's the best solution, hence opening this to discuss.

The problem is that during set_custom_element_data, get_setters is the only check done to differentiate between setting the value as a prop (has a setter) or as an attribute (doesn't have a setter).

The solution is to take into account whether or not the custom element is already registered, and defer getting (and caching) its setters until then. Instead, fall back to a "an object is always set as a prop" heuristic.
  • Loading branch information
dummdidumm authored Dec 10, 2024
1 parent cb5734a commit a2539cf
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 4 deletions.
5 changes: 5 additions & 0 deletions .changeset/sharp-shoes-look.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: take into account registration state when setting custom element props
20 changes: 16 additions & 4 deletions packages/svelte/src/internal/client/dom/elements/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ export function set_xlink_attribute(dom, attribute, value) {
}

/**
* @param {any} node
* @param {HTMLElement} node
* @param {string} prop
* @param {any} value
*/
Expand All @@ -216,10 +216,21 @@ export function set_custom_element_data(node, prop, value) {
set_active_reaction(null);
set_active_effect(null);
try {
if (get_setters(node).includes(prop)) {
if (
// Don't compute setters for custom elements while they aren't registered yet,
// because during their upgrade/instantiation they might add more setters.
// Instead, fall back to a simple "an object, then set as property" heuristic.
setters_cache.has(node.nodeName) || customElements.get(node.tagName.toLowerCase())
? get_setters(node).includes(prop)
: value && typeof value === 'object'
) {
// @ts-expect-error
node[prop] = value;
} else {
set_attribute(node, prop, value);
// We did getters etc checks already, stringify before passing to set_attribute
// to ensure it doesn't invoke the same logic again, and potentially populating
// the setters cache too early.
set_attribute(node, prop, value == null ? value : String(value));
}
} finally {
set_active_reaction(previous_reaction);
Expand Down Expand Up @@ -384,8 +395,9 @@ function get_setters(element) {
var setters = setters_cache.get(element.nodeName);
if (setters) return setters;
setters_cache.set(element.nodeName, (setters = []));

var descriptors;
var proto = get_prototype_of(element);
var proto = element; // In the case of custom elements there might be setters on the instance
var element_proto = Element.prototype;

// Stop at Element, from there on there's only unnecessary setters we're not interested in
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { flushSync } from 'svelte';
import { test } from '../../assert';

const tick = () => Promise.resolve();

// Check that rendering a custom element and setting a property before it is registered
// does not break the "when to set this as a property" logic
export default test({
async test({ assert, target }) {
target.innerHTML = '<custom-element></custom-element>';
await tick();
await tick();

const ce_root = target.querySelector('custom-element').shadowRoot;

ce_root.querySelector('button')?.click();
flushSync();
await tick();
await tick();

const inner_ce_root = ce_root.querySelectorAll('set-property-before-mounted');
assert.htmlEqual(inner_ce_root[0].shadowRoot.innerHTML, 'object|{"foo":"bar"}');
assert.htmlEqual(inner_ce_root[1].shadowRoot.innerHTML, 'object|{"foo":"bar"}');
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<svelte:options customElement="custom-element" />

<script lang="ts">
import { onMount } from 'svelte';
class CustomElement extends HTMLElement {
constructor() {
super();
this.attachShadow({ mode: 'open' });
Object.defineProperty(this, 'property', {
set: (value) => {
this.shadowRoot.innerHTML = typeof value + '|' + JSON.stringify(value);
}
});
}
}
onMount(async () => {
customElements.define('set-property-before-mounted', CustomElement);
});
let property = $state();
</script>

<button onclick={() => (property = { foo: 'bar' })}>Update</button>
<!-- one that's there before it's registered -->
<set-property-before-mounted {property}></set-property-before-mounted>
<!-- and one that's after registration but sets property to an object right away -->
{#if property}
<set-property-before-mounted {property}></set-property-before-mounted>
{/if}

0 comments on commit a2539cf

Please sign in to comment.