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

feat: add support for bind getter/setters #14307

Merged
merged 28 commits into from
Dec 8, 2024
Merged

feat: add support for bind getter/setters #14307

merged 28 commits into from
Dec 8, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Nov 15, 2024

WIP

closes #3937

Copy link

changeset-bot bot commented Nov 15, 2024

🦋 Changeset detected

Latest commit: 0a7608e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-14307-svelte.vercel.app/

this is an automated message

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14307

@Leonidaz
Copy link

Nice!

One piece of feedback, I think it would be helpful to be able to define a getter and setter outside the bind:, inside <script>, for a cleaner markup

Maybe in addition to or instead of allowing using an object with get / set as properties or an array of 2 functions. And, at the same time allow bind: to a const if this syntax is used.

using an object with get and set functions:

<script>
	import Child from './Child.svelte';
	let a = $state(0);

	const accessorsA = {
		get () {return  a},
		set (v) {
			console.log('a', v);
			a = v;
		}
	};
</script>

<button onclick={() => a++}>a: {a}</button>

<Child
	bind:a={accessorsA}
/>

using an array with 2 functions

<script>
	import Child from './Child.svelte';
	let a = $state(0);

	const accessorsA = [
		() => a,
		(v) => {
			console.log('a', v);
			a = v;
		}
	]
</script>

<button onclick={() => a++}>a: {a}</button>

<Child
	bind:a={accessorsA}
/>

Currently it's doable of course if you define 2 functions or use a destructuring assignment from an array but it's kind of annoying:

<script>
	import Child from './Child.svelte';
	let a = $state(0);

	let [getA, setA] = [
		() => a,
		(v) => {
			console.log('a', v);
			a = v;
		}
	]
	
	// or
	
	getA = () => a;
	setA = (v) => {
	    console.log('a', v);
	    a = v;
	};
</script>

<button onclick={() => a++}>a: {a}</button>

<Child
	bind:a={getA, setA}
/>

Personally, as far as being very intentional and explicit, I would prefer an object with a get and set as properties.


Also, currently getting a Svelte warning ownership_invalid_mutation if using a nested object for binding with get and set.

If you change the value in the input, this warning is shown:

ownership_invalid_mutation Child.svelte mutated a value owned by App.svelte. This is strongly discouraged. Consider passing values to child components with `bind:`, or use a callback instead

Playground: ownership_invalid_mutation

If the getter and setter in the parent only expose a particular property of the nested object then it works without a warning,

Only access a nested property vs the whole object

@Rich-Harris
Copy link
Member

I mean you can do bind:value={binding.get, binding.set} or bind:value={binding[0], binding[1]}, right? So I'm not sure there's an advantage to mandating a particular 'shape', even if it would work, which it doesn't because it's ambiguous in the case of components (even if you detect at runtime whether something matches the shape, which is overhead in and of itself, you can't know definitively that the { get, set } pair isn't itself the bound value).

Doing it this way — with syntax — is better:

  • it's unambiguous
  • it allows the compiler to do its job (instead of forcing overhead on the runtime)
  • it lets people use whatever shape makes sense for them (if I happen to have an object with read and write methods that was given to me by some library, I don't have to wrap it)
  • it's easy to add handlers inline, which is where I'd expect to be using this most of the time

The warning looks like a bug, will investigate

@Rich-Harris
Copy link
Member

fixed the warning by hoisting the code that adds the child component as an owner of the state. to do that I changed the internal representation from an [Expression, Expression] to SequenceExpression, which makes sense anyway as it simplifies a bunch of other stuff

@Leonidaz
Copy link

I mean you can do bind:value={binding.get, binding.set} or bind:value={binding[0], binding[1]}, right? So I'm not sure there's an advantage to mandating a particular 'shape', even if it would work, which it doesn't because it's ambiguous in the case of components (even if you detect at runtime whether something matches the shape, which is overhead in and of itself, you can't know definitively that the { get, set } pair isn't itself the bound value).

Doing it this way — with syntax — is better:

  • it's unambiguous
  • it allows the compiler to do its job (instead of forcing overhead on the runtime)
  • it lets people use whatever shape makes sense for them (if I happen to have an object with read and write methods that was given to me by some library, I don't have to wrap it)
  • it's easy to add handlers inline, which is where I'd expect to be using this most of the time

yeah, great points, especially about the runtime overhead, definitely compile time is the best option.

As it stands, using it this way is kind of an unusual syntax for Svelte but I think it's fine.

I guess making a syntax inline with get and set required should (my assumption) work with the compiler <Component bind:a={ {get: functionGet, set: functionSet } } /> but it's kind of weird too.

@Rich-Harris
Copy link
Member

It's weird (unnecessarily verbose) but also misleading, since it looks like you should be able to hoist the object

dummdidumm added a commit to sveltejs/language-tools that referenced this pull request Nov 22, 2024
@Rich-Harris Rich-Harris marked this pull request as ready for review December 8, 2024 12:28
@Rich-Harris Rich-Harris merged commit 5771b45 into main Dec 8, 2024
11 checks passed
@Rich-Harris Rich-Harris deleted the bind-get-set branch December 8, 2024 12:28
@github-actions github-actions bot mentioned this pull request Dec 8, 2024
@AlbertMarashi
Copy link

AlbertMarashi commented Dec 9, 2024

@Rich-Harris this is great, but I seem to be getting some IDE errors, perhaps this stuff hasn't been added to the latest svelte language tools or other packages that the IDE depends on?

@dummdidumm
Copy link
Member

the eslint plugin is not up to date yet, the other tools should be (assuming you bump to their latest version)

@mskocik
Copy link

mskocik commented Dec 14, 2024

I suggest adding demo from "Advent of Svelte" blog post to documentation itself. I believe it makes things obvious for the reader.

I had to look up the demo few times myself :)

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

Successfully merging this pull request may close these issues.

Input modifiers
6 participants