Skip to content

Conversation

@dummdidumm
Copy link
Member

bind:this etc only go upwards, and depending on the use case it's fine to connect it to a non-reactive variable

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

`bind:this` etc only go upwards, and depending on the use case it's fine to connect it to a non-reactive variable
@changeset-bot
Copy link

changeset-bot bot commented Jul 23, 2024

🦋 Changeset detected

Latest commit: be331d7

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

Is bidirectionality the right criterion? Feels a bit weird to exclude <svelte:window bind:innerWidth={pojo.value}> — if I'm not reacting to changes in innerWidth then what's the point of the binding?

If we want a basis on which to exclude bind:this, then something like 'is only updated once' would surely make more sense (though it would need to be combined with a 'is not in a block' check).

Side-note: I thought we were already warning on non-reactive Identifier bindings at compile time, but it seems that we're doing no such thing. Did I hallucinate that or did we stop doing it?

@dummdidumm
Copy link
Member Author

dummdidumm commented Jul 24, 2024

Is bidirectionality the right criterion? Feels a bit weird to exclude <svelte:window bind:innerWidth={pojo.value}> — if I'm not reacting to changes in innerWidth then what's the point of the binding?

<script>
  let online;
  let innerWidth;

  function printStuff() {
    if (!online) alert('cant do this right now')

    let size = innerWidth / ...;
    createPicture(size);
    print();
  }
</script>

<svelte:window bind:online bind:innerWidth />

Similar examples can be created for the other readonly bindings.

Side note: the above currently creates a compiler warning, is that correct? I feel like it's a bit overzealous.

If we want a basis on which to exclude bind:this, then something like 'is only updated once' would surely make more sense (though it would need to be combined with a 'is not in a block' check).

This PR started out as "don't warn on bind:this" and then I thought of the more general one-way-bindings and felt it should apply there, too.

Side-note: I thought we were already warning on non-reactive Identifier bindings at compile time, but it seems that we're doing no such thing. Did I hallucinate that or did we stop doing it?

We are warning on it?

@trueadm
Copy link
Contributor

trueadm commented Jul 24, 2024

We don't warn on this

@dummdidumm
Copy link
Member Author

Yeah but that's correct, because it doesn't need to be reactive, it would be (was! we changed this) very annoying to have a warning in this case.

@trueadm
Copy link
Contributor

trueadm commented Jul 24, 2024

@dummdidumm I mean, I'd argue that it should be though. It keeps it consistent with the rest of our reactivity game.

@Rich-Harris
Copy link
Member

I would write the above example like this:

<script>
  function printStuff() {
    if (!navigator.onLine) alert('cant do this right now')

    let size = innerWidth / 1;
    createPicture(size);
    print();
  }
</script>

The only reason to use a binding is because you want something to be reactive. bind:this is a bit of a special case because it updates exactly once, and it does so immediately before the first effects run (e.g. onMount). I don't think there's any other binding with those characteristics.

@dummdidumm
Copy link
Member Author

Adjusted it so it's only silencing the "bind:this not in a block" case

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.

4 participants