extract onchange callbacks from options

[Rich-Harris]

This is a PR against #15069. My thesis is that it's quite unlikely we'll end up with more state options, and until we do we can make things more efficient by just passing the callbacks around rather than creating a bunch of objects and cloning them

cc @paoloricciuti

[changeset-bot]

⚠️ No Changeset found

Latest commit: 976ff68

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[github-actions]

Playground

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

[paoloricciuti]

I like the syntax more but I'm a bit scared because this will lock us into not adding any new option before svelte 6 unless we go the

$state(0, ()=>{}, {...});

Or with variadic arguments.

That said I agree is probably unlikely we would need to add any new option (I can't think of anything) and we could always resort to a new rune.

[Ocean-OS]

Found a regression: https://svelte.dev/playground/reactive-assignments?version=pr-15579#H4sIAAAAAAAAA21P22rDMAz9FSEKTVjI1tes6Rj9g73WhbqOmpo6cqiVshHy78O5bHvYk44O5yL1yLohLPCDtBH7INAh2JobYgmQUGWFqhQzvFhHAYtDj_LVRkMkMFvs722bhwc5idxZB_qPN54l5mKB22DutpWdYiWOBPS9DlDCIe5KXrJp9tNQ4tlcNdeUpD-UEuM5eEe583Vy2vuOBWyAVW8iHE7pohwmMI7jUjiKoIRVEC2U5HkeT0hfFUfFpWMj1jNcNVeO9s6aW5LCXD55n0rYRHkM3j7__sPbcyfiGTyb6Cv7PyHD-PEIqYL51Ej180FlCRt4g7XYhtZQTCCsx44pd6cYMxT6FCzk3tFwHL4BXGezK8YBAAA=

[paoloricciuti]

Found a regression: https://svelte.dev/playground/reactive-assignments?version=pr-15579#H4sIAAAAAAAAA21P22rDMAz9FSEKTVjI1tes6Rj9g73WhbqOmpo6cqiVshHy78O5bHvYk44O5yL1yLohLPCDtBH7INAh2JobYgmQUGWFqhQzvFhHAYtDj_LVRkMkMFvs722bhwc5idxZB_qPN54l5mKB22DutpWdYiWOBPS9DlDCIe5KXrJp9tNQ4tlcNdeUpD-UEuM5eEe583Vy2vuOBWyAVW8iHE7pohwmMI7jUjiKoIRVEC2U5HkeT0hfFUfFpWMj1jNcNVeO9s6aW5LCXD55n0rYRHkM3j7__sPbcyfiGTyb6Cv7PyHD-PEIqYL51Ej180FlCRt4g7XYhtZQTCCsx44pd6cYMxT6FCzk3tFwHL4BXGezK8YBAAA=

In this pr the second argument should be a function

[paoloricciuti]

Oh wait I'm super wrong and I completely misunderstood the PR 😑

I shouldn't check those on the phone lol

[Rich-Harris]

Huh, I'm amazed we currently allow spread arguments to $state. I think we should just disallow that

[paoloricciuti]

But yeah I think this basically forces the argument to be statically analyzable to pass the function which we decided against no?

[Rich-Harris]

I think there's a meaningful distinction between allowing spread elements (which no-one would ever need) and allowing arbitrary expressions for the second argument (which is potentially useful)

[paoloricciuti]

Yeah I think we can just disallow spread

[paoloricciuti]

Would that be considered a breaking tho? I don't think no one in their right mind ever did this but... 😁

[Ocean-OS]

I think that should be a breaking change just in case.

[paoloricciuti]

The alternative is to unwrap the rest

$.state(args[0], args[1].onchange);

[Rich-Harris]

I mean we could do that but if someone has actually written that code then god help them. I don't think this rises to the level of 'breaking change'

[paoloricciuti]

I mean we could do that but if someone has actually written that code then god help them. I don't think this rises to the level of 'breaking change'

I have the change locally...should i push it?

[paoloricciuti]

i pushed it...we can always revert if we don't want it

[Rich-Harris]

I think it adds a lot of needless complexity, I'd much rather we just prohibit spread arguments with runes (except perhaps $inspect(...stuff)

[paoloricciuti]

I think it adds a lot of needless complexity, I'd much rather we just prohibit spread arguments with runes (except perhaps $inspect(...stuff)

I mean...i think we are safe 😄😄😄

[paoloricciuti]

Opened a PR to prevent spreads

#15585

[adiguba]

Hi,

My two cents : I think that the compiler should throw an error when options contains incorrect properties :

let name = $state('world', {
	xxx: 42 // should throw "Invalid option for $state()"
});

[paoloricciuti]

Hi,

My two cents : I think that the compiler should throw an error when options contains incorrect properties :

let name = $state('world', {
	xxx: 42 // should throw "Invalid option for $state()"
});

We removed a bunch of this compiler errors deferring the work to typescript

[Rich-Harris]

Since we're now disallowing spreads I'll go ahead and merge this