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

Inconsistent bind: behaviour with spread objects on components versus elements #14839

Open
qwuide opened this issue Dec 26, 2024 · 10 comments
Open

Comments

@qwuide
Copy link

qwuide commented Dec 26, 2024

Describe the bug

The behaviour of bind: differs when spreading an object onto a component versus an element.

When spreading an object onto a component containing a key with both a get and a set accessor, a binding is established for that key.

However, when spreading the same object onto an element, the binding is not established. Explicitly declaring the binding (e.g. bind:value={obj.value}) works, though.

I'm not sure what the intended behaviour is, but I think it should be consistent for both components and elements.

Reproduction

Playground

Logs

No response

System Info

...

Severity

annoyance

@Ocean-OS
Copy link
Contributor

This is simply how object spreading works, it doesn't preserve getters and setters.

@qwuide
Copy link
Author

qwuide commented Dec 27, 2024

This is simply how object spreading works, it doesn't preserve getters and setters.

That’s true, and I didn’t consider this. However, when looking at the compiled output, it seems that for components, a Svelte-specific function, $.spread_props (client and server code), is used to handle the spreading, and this function appears to actively preserve the accessors. In contrast, for elements, a regular spread is used, which, as you mentioned, does not preserve the accessors and might explain why the bindings are not established.

That said, I believe the behaviour should be consistent, as there is no apparent difference in how the spread is written in the code from a user perspective.

@Leonidaz
Copy link

That said, I believe the behaviour should be consistent, as there is no apparent difference in how the spread is written in the code from a user perspective.

That's how it has always worked. You've had to use bind: in order to get reactivity with html elements. Otherwise, you can provide event handlers e.g. oninput / onchange to update variables from element values.

Also, for html elements, you're simply providing attributes + event handlers. It's not like you can use composition to access them in some inner elements.

Svelte should not just automatically provide bindings as it doesn't know the code's intentions, whether you're trying to establish a binding or simply specify attributes and their values. That's why you specify either bind: or event handlers to make the connection explicit.

@qwuide
Copy link
Author

qwuide commented Dec 27, 2024

That said, I believe the behaviour should be consistent, as there is no apparent difference in how the spread is written in the code from a user perspective.

That's how it has always worked. You've had to use bind: in order to get reactivity with html elements. Otherwise, you can provide event handlers e.g. oninput / onchange to update variables from element values.

Also, for html elements, you're simply providing attributes + event handlers. It's not like you can use composition to access them in some inner elements.

Svelte should not just automatically provide bindings as it doesn't know the code's intentions, whether you're trying to establish a binding or simply specify attributes and their values. That's why you specify either bind: or event handlers to make the connection explicit.

I think you’ve misunderstood the issue. Of course, I understand that bind: is required to achieve reactivity, but that’s not the point here. I’m also not suggesting that Svelte should automatically provide bindings — only that bind: with spread should work consistently between components and elements.

@Leonidaz
Copy link

That said, I believe the behaviour should be consistent, as there is no apparent difference in how the spread is written in the code from a user perspective.

That's how it has always worked. You've had to use bind: in order to get reactivity with html elements. Otherwise, you can provide event handlers e.g. oninput / onchange to update variables from element values.
Also, for html elements, you're simply providing attributes + event handlers. It's not like you can use composition to access them in some inner elements.
Svelte should not just automatically provide bindings as it doesn't know the code's intentions, whether you're trying to establish a binding or simply specify attributes and their values. That's why you specify either bind: or event handlers to make the connection explicit.

I think you’ve misunderstood the issue. Of course, I understand that bind: is required to achieve reactivity, but that’s not the point here. I’m also not suggesting that Svelte should automatically provide bindings — only that bind: with spread should work consistently between components and elements.

I'm confused then what this is about. Is this about your proposed syntax <input {...inputProps}>? Otherwise, please provide the syntax that you're suggesting.

@qwuide
Copy link
Author

qwuide commented Dec 27, 2024

I'm confused then what this is about. Is this about your proposed syntax <input {...inputProps}>? Otherwise, please provide the syntax that you're suggesting.

Sorry for the confusion! This is not about proposing a new syntax. The issue I'm highlighting is the inconsistency in behaviour when using bind: with spread between components and native elements.

Currently, when spreading an object with get/set accessors to a component, the accessors become reactive. However, this doesn’t happen for native elements. To better illustrate this, please check out the example I provided. Try entering some text into the input fields, and you’ll see that reactivity works for the component but not for the native input.

@Ocean-OS
Copy link
Contributor

Ocean-OS commented Dec 27, 2024

I'm confused then what this is about. Is this about your proposed syntax <input {...inputProps}>? Otherwise, please provide the syntax that you're suggesting.

It's about how if you use a pattern like this (which is a supported and documented pattern):

<script>
let thing = $state("");
let elementProps = {
    get value() {
        return thing;
    },
    set value(v) {
        thing = v;
    }
};
</script>
<input {...elementProps} />
<Input {...elementProps} />

The accessor gets treated as a binding if you use the props on a component, which makes sense, because Svelte turns component bindings into the same exact pattern. However, if you do this on a regular element (<input>, in this case), the binding does not work, because Svelte has specific functions for regular element bindings. The best solution I can think of would be to use $.spread_props on regular elements when compiling, which may require some modifications of $.set_attributes as well, so as to better interpret getters and setters.

@Leonidaz
Copy link

I'm confused then what this is about. Is this about your proposed syntax <input {...inputProps}>? Otherwise, please provide the syntax that you're suggesting.

It's about how if you use a pattern like this (which is a supported and documented pattern):

<script>
let thing = $state("");
let elementProps = {
    get value() {
        return thing;
    },
    set value(v) {
        thing = v;
    }
};
</script>
<input {...elementProps} />
<Input {...elementProps} />

The accessor gets treated as a binding if you use the props on a component, which makes sense, because Svelte turns component bindings into the same exact pattern. However, if you do this on a regular element (<input>, in this case), the binding does not work, because Svelte has specific functions for regular element bindings. The best solution I can think of would be to use $.spread_props on regular elements when compiling, which may require some modifications of $.set_attributes as well, so as to better interpret getters and setters.

But that's I what meant with this syntax, Svelte should not try to figure out the code's intention.

Svelte should not just automatically provide bindings as it doesn't know the code's intentions, whether you're trying to establish a binding or simply specify attributes and their values. That's why you specify either bind: or event handlers to make the connection explicit.

It's one thing to deal with props on components which are basically function parameters and visible to svelte's code vs defining attributes on elements which could come from anywhere. How would it know whether it should be an attribute or a binding? Especially if the object contains multiple properties.

I agree that it's possible but then Svelte needs to be fully aware of which attributes each element supports, which are writable vs readable, etc. And also, it needs visibility into the object structure to see that there is setter and a getter to create a binding. In the same component it can be analyzed but what if it's passed in, that ability disappears. Yes, you can use getOwnPropertyDescriptors but what if it's an instance of a class, how to even identify that and then call getOwnPropertyDescriptors on class proto... It gets super hairy.

@Ocean-OS
Copy link
Contributor

But that's I what meant with this syntax, Svelte should not try to figure out the code's intention.

I don't see how else a getter and setter would be interpreted... a getter and setter prop would only make sense if it were a binding, would it not?

It's one thing to deal with props on components which are basically function parameters and visible to svelte's code vs defining attributes on elements which could come from anywhere. How would it know whether it should be an attribute or a binding? Especially if the object contains multiple properties.

I agree that it's possible but then Svelte needs to be fully aware of which attributes each element supports, which are writable vs readable, etc. And also, it needs visibility into the object structure to see that there is setter and a getter to create a binding. In the same component it can be analyzed but what if it's passed in, that ability disappears. Yes, you can use getOwnPropertyDescriptors but what if it's an instance of a class, how to even identify that and then call getOwnPropertyDescriptors on class proto... It gets super hairy.

I don't quite understand what you mean by "what if it's an instance of a class", since couldn't it just do something like this?

function find_bindings(props) {
    let keys = Object.keys(props); 
    let bindings = new Map;
    for (let index = 0; index < keys.length; i++) {
        let descriptor = Object.getOwnPropertyDescriptor(props, key); 
        if (descriptor && descriptor.get && descriptor.set) {
            bindings.set(key, descriptor);
        }
    }
    return bindings;
}

If you mean that the spread props would be a class, I'm not sure if that's even officially supported in Svelte. It seems to work, however, and the concern you had about prototype chains seems nonexistent (check logs).

@Leonidaz
Copy link

I don't see how else a getter and setter would be interpreted... a getter and setter prop would only make sense if it were a binding, would it not?

Maybe your intention is only to provide an attribute. Also, inside the component Svelte can see a getter and setter but not necessarily if the object comes from the outside, e.g. import from a file, etc.

I don't quite understand what you mean by "what if it's an instance of a class", since couldn't it just do something like this?

function find_bindings(props) {
    let keys = Object.keys(props); 
    let bindings = new Map;
    for (let index = 0; index < keys.length; index++) {
        let descriptor = Object.getOwnPropertyDescriptor(props, keys[index]); 
        if (descriptor && descriptor.get && descriptor.set) {
            bindings.set(key, descriptor);
        }
    }
    return bindings;
}

this function would not work directly with a class instance if a property was defined with a getter and a setter, you'd have to check the prototype.

	class Props {
		#align="center";

		get align() {
			return this.#align;
		}

		set align(v) {
			this.#align = v;
		}
	}

If you mean that the spread props would be a class, I'm not sure if that's even officially supported in Svelte. It seems to work, however, and the concern you had about prototype chains seems nonexistent (check logs).

The spread itself would definitely work. I was referring to trying to figure out if each property has a getter and a setter in order to decide whether it's going to be a bind: vs a simple attribute. For which you'd have to check the prototype, getPrototypeOf, etc.

The other thing is this kind of analysis would also have to be done at runtime because Svelte cannot see outside of the component if an object comes from the outside. Now it's done at compile time.

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

No branches or pull requests

3 participants