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

Spread operator doesn't work with "form" and "list" attributes #3681

Closed
imkremen opened this issue Oct 10, 2019 · 2 comments · Fixed by #3687
Closed

Spread operator doesn't work with "form" and "list" attributes #3681

imkremen opened this issue Oct 10, 2019 · 2 comments · Fixed by #3687
Labels

Comments

@imkremen
Copy link

Describe the bug
I'll try set list and form with spread operator:

let [type, form, list] = ['text', 'myID', 'listID'];
let props = {type, form, list};

It works in regular case:

<input type={type} form={form} list={list}>

But fails with spread operator:

<input {...props}>

Logs
Firefox: setting getter-only property "form"
Chrome: Cannot assign to read only property 'form' of object '#<HTMLInputElement>'

And if we remove "form" attribute:
Firefox: setting getter-only property "list"
Chrome: Cannot assign to read only property 'list' of object '#<HTMLInputElement>'

To Reproduce
REPL

@imkremen imkremen changed the title Spread operator doesn't work with "forms" and "list" attributes Spread operator doesn't work with "form" and "list" attributes Oct 10, 2019
@Conduitry
Copy link
Member

This looks like another manifestation for an overarching problem which might not actually have its own issue (just a few related open issues) - namely, that we don't know whether each prop that's getting spread should be set as an attribute or as a property on the element. For non-spread props, we can look this stuff up at compile time, and just emit the correct code. I don't think we have a really good way to handle this for spread props (where this needs to be determined at runtime) without shipping a big list of properties vs attributes in the compiled code, which we'd really like to avoid doing.

The helper we're using right now for this is:

export function set_attributes(node: Element & ElementCSSInlineStyle, attributes: { [x: string]: string }) {
	for (const key in attributes) {
		if (key === 'style') {
			node.style.cssText = attributes[key];
		} else if (key in node) {
			node[key] = attributes[key];
		} else {
			attr(node, key, attributes[key]);
		}
	}
}

We set an attribute if one is available on the DOM object, otherwise we use an attribute. This breaks when it's a read-only property. It also doesn't handle when the property has a different name than the attribute, apart from the one special case of style.

I recall it being suggested somewhere I can't find that one possibility would be to fall back to setting it as an attribute if node[key] = attributes[key]; fails. This might be the best we can reasonably do. This definitely is an area that needs work.

@Conduitry Conduitry added the bug label Oct 10, 2019
@Conduitry
Copy link
Member

A good idea from @pngwn: Check for Object.getOwnPropertyDescriptor(node.__proto__, key).set rather than key in node. These read-only properties don't actually have setters, which we can check for at runtime. Actually, we should probably do Object.getOwnPropertyDescriptors(node.__proto__) once at the beginning, and then do the lookups in that, which I imagine would be faster.

pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
pngwn added a commit to pngwn/svelte that referenced this issue Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants