fix: warn instead of error when binding a non-existent prop

[oscard0m]

Svelte 5 rewrite

closes #13368

@paoloricciuti and I paired on this.

In situations like the one described in the issue is fine to warn instead of error out but we are up to discuss this.

---

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• Prefix your PR title with feat:, fix:, chore:, or docs:.
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests and linting

• Run the tests with pnpm test and lint the project with pnpm lint

[changeset-bot]

🦋 Changeset detected

Latest commit: c6dc740

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

[dummdidumm]

Personal opinion: We could go one step further and emit nothing whatsoever in this case, similar to how we defer "this prop doesn't exist on this component" errors to TypeScript now. Erroring on a prop that is not bindable or a component export that is not bindable like this are different to "doesn't exist at all" to me.

[paoloricciuti]

Personal opinion: We could go one step further and emit nothing whatsoever in this case, similar to how we defer "this prop doesn't exist on this component" errors to TypeScript now. Erroring on a prop that is not bindable or a component export that is not bindable like this are different to "doesn't exist at all" to me.

Yeah I guess we can...maybe the one difference between this and a normal component is that here is a bit easier to trick typescript because you are kinda already tricking it creating the array so maybe have a runtime warn could be better if you are expecting that component to have the props and be bindable? I'm not opposed to removing it tho

[paoloricciuti]

Also in this case TS doesn't help 😦

[paoloricciuti]

@dummdidumm sorry for the double ping but i wanted to restart the discussion on this since it's been a while 😄

[rgon]

For the use case I brought up in #13368 (prop overloading), it definitely should not be a runtime warning, since runtime warnings cannot be ignored with // svelte-ignore ..., littering the console unnecessarily on a genuine use case. Also, it intuitively makes sense to keep TS and runtime warnings/errors as 1-1 as possible.

Also, I believe the language-tools are working correctly, and thus no runtime warning should happen:

Here's a repl with @paoloricciuti 's example: REPL. For clarification, as of now, it does not raise any TS error, which is correct:

And as expected, if we add a the non-bindable c prop to A.svelte REPL 2 we do get the error we expect

Cannot use 'bind:' with this property. It is declared as non-bindable inside the component.
To mark a property as bindable: 'let { c = $bindable() } = $props()'ts(2322)

So the language tools are working correctly.

IMHO, this 'error' should simply be ignored by svelte itself, leaving it to svelte-check and the such, which at this time have no issues.

[paoloricciuti]

I think it's fine to not do anything then... @oscard0m do you want to work on this on your own? Should we pair?

[oscard0m]

I think it's fine to not do anything then... @oscard0m do you want to work on this on your own? Should we pair?

Let me give it a try! I pushed the changes, could you validate them @paoloricciuti?

[paoloricciuti]

Looks good to me @dummdidumm if you want to give it a final look just because I paired on it that would be better.

[Rich-Harris]

If we're happy with the type error, do we need to keep validate_prop_bindings at all? Should we just get rid of it altogether? Opened #14946 as a potential alternative

[dummdidumm]

So basically taking my comment of not erroring in this case (#13416 (comment)) even further, by defering to TS completely? We could do that now since the IDE catches this mistake. I gotta play around with the version where it's gone completely though to see what exactly happens if you bind to something that can't be bound - if you're not using TS it could be a bit confusing, then again the same is true for typos etc.

Update: So I played around with this a bit and I can see advantages and disadvantages to it, depending on your view point:

• advantage: if someone forgot to mark something as bindable, or someone really really want to use them that way they can force the binding even if it's not declared as such
• disadvantage 1: you don't have the guarantee anymore that someone can't bind to your prop. To be fair that isn't guaranteed either today in prod, because the validation only happens at dev time.
• disadvantage 2: in case you're not using TS, you don't get the info that you shouldn't bind to this. Then again, you don't get any info about props spelled wrong either, in other words you gotta look up the component code either way.

In other words $bindable more or less becomes purely a TypeScript hint. Given that it worked that way in prod, too, I think I'm in favor of removing the validation.

[Rich-Harris]

advantage: if someone forgot to mark something as bindable, or someone really really want to use them that way they can force the binding even if it's not declared as such

In #14946 reassigning a prop will not invoke the binding if it's not bindable, precisely so that you can't do this against the component author's wishes. I didn't think about the mutation case though. A component shouldn't be mutating non-bindable props (since either it doesn't own them, or it's uselessly mutating a non-reactive fallback value), and allowing people to 'fix' the resulting ownership validation warning by adding bind: on the consumer side without making it bindable feels wrong... though I suppose this is covered by the type error anyway?

In other words I think I agree, just trying to think through all the ramifications

[dummdidumm]

A component shouldn't be mutating non-bindable props

In general yes, but it could be on purpose, say you pass a class instance with setters which do something, in which case management of the object is responsibility to the class, not the parent component.

<script>
  class Foo {
     set bar(b) { ... }
  }
  const foo = new Foo();
</script>

<Child {foo} />
```

[Rich-Harris]

merged #14946 so i'll close this — thanks