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

WIP: Add $effect.sync rune #12959

Closed
wants to merge 2 commits into from
Closed

Conversation

mimbrown
Copy link
Contributor

@mimbrown mimbrown commented Aug 21, 2024

WORK IN PROGRESS

This PR adds a new $effect.sync rune for synchronous effects.

Based on this off-topic conversation thread that I caused.

Changes

  • add support for sync effects to the runtime.
  • add $effect.sync as a supported rune to the compiler.

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

Copy link

changeset-bot bot commented Aug 21, 2024

⚠️ No Changeset found

Latest commit: 75dc60e

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

@mimbrown
Copy link
Contributor Author

@trueadm based on our conversation I thought it'd be easier to explore with a PR. I saw that $inspect effects are already synchronous, so I piggybacked off that and brought it out from behind the DEV checks. So far I just added runtime support. As far as I understand, this should trigger properly in all cases where $inspect would trigger properly.

If this is not going to work, I'd love to understand why. If it could work and there's interest, I will continue with compiler updates, tests, docs, etc.

@trueadm
Copy link
Contributor

trueadm commented Aug 22, 2024

Inspect effects don't mutate anything, so they're harmless to the reactive graph.

@mimbrown
Copy link
Contributor Author

I see how the code I had could cause issues if the sync effects scheduled other sync effects. I pushed something I think should fix that.

Here are the main issues you've raised, as I've understood them:

Overfiring

Sync effects could easily overfire.

Simple example:

let a = $state(0);
let b = $state(1);

$effect.sync(() => {
  console.log('Sum is', a + b);
});

function increment() {
  // will trigger the effect twice in a row, one for each change
  a += 1;
  b += 1;
}

More subtle example:

let a = $state(0);

$effect.sync(() => {
  if (a % 2 !== 0) {
    a -= 1;
  }
});

$effect.sync(() => {
  console.log(a);
});

// Should cause the first effect to fire twice.
//
// Should also cause the second effect to fire twice.
// If I've traced it correctly, both runs would log `2`
// and the `3` would never be logged. If the effects
// were ordered differently, it would probably log `3`
// and then `2`.
a += 3;

I personally don't think that the overfiring is a big deal. It's an issue if you're using sync effects generally but I don't think it's a big problem in the situations where you would actually need to reach for them. Could be wrong.

Out-of-order triggering

You mentioned that there was a specific order the effects are supposed to trigger in. I don't understand it all, but I'm comfortable saying that "sync effects are not guaranteed to run in a specific order. If you're depending on multiple sync effects running in a particular order, please reconsider your use case."

In reality, I expect that the sync effects would trigger in the order that they are registered, depth-first (i.e. any sync effects triggered by the first effect would run before the other sync effects).

Possible breakdown of the reactive graph entirely?

I haven't been able to come up with how this could be, so if you have an example please share.

Obviously sync effects have pitfalls, and there's a reason they're not the default. But I think it can be exposed as an option, with the appropriate caveats.

@trueadm
Copy link
Contributor

trueadm commented Aug 23, 2024

Out-of-order triggering

$inspect also suffers from this issue, but given you're using it purely for debugging, people are happy to work around its issues. For example it fires out of order. Remove the $inspect or change for an effect and the issue goes away.

@mimbrown
Copy link
Contributor Author

mimbrown commented Aug 23, 2024

There's no question that sync effects have more footguns and less guarantees. But I believe they should be an available tool for the occasional scenario where they're useful. @trueadm do you disagree?

EDIT: nice example by the way, that's helpful to understand some of the issues

@trueadm
Copy link
Contributor

trueadm commented Aug 23, 2024

@mimbrown We used to have an unofficial sync effect but it was far too buggy and caused so many issues because of the async effects having a different timing and thus co-ordination failed. There are compounding issues, but realistically I don't think we re-introduce them again – not unless we want to remove transitions/animations from Svelte – because sync effects cannot exist when you have transitions working on a different timing schedule.

As you can see $inspect is full of gotchas too, but at least you never mutate anything in there – if you did, you'd also cause anything with transitions to completely bork. You can force everything to be sync with flushSync to have the entire system run as sync though, that will work fine, you just can't run two systems and expect them to play ball together.

@mimbrown
Copy link
Contributor Author

you just can't run two systems and expect them to play ball together

I get that, if you're ever trying to "coordinate" sync and async effects, you're gonna have a bad time. The use cases I have in mind don't want to coordinate with any async effects.

The reason this came up (as you know) was because of the example Rich posted about using an effect to emulate the just-removed $state.link rune. The problem (as people pointed out) is there can be unexpected syncing behaviors when using an effect to keep states up-to-date with each other.

let a = $state(0);
let b = $state(a);

$effect(() => {
  b = a;
});

a = 1;
console.log(b); // expect 1, but actually 0

This would be a valid use case for $effect.sync. And it doesn't cause issues (unless I'm missing something?).
I think of sync effects as, "every time I see a line that updates/mutates a dependency of the sync effect, then take the whole body of the sync effect and stick it after that line." That's practically what happens. And in the above example that's exactly what I want to happen.

You could even pull a react and call it $effect.dangerouslyRunSynchronously, I don't know. It's one of those "if you don't know that you need it, then you don't need it, but if you do know then you can use it."

@Antonio-Bennett
Copy link

I don’t know if it is worth going down this rabbit hole of trying to add a sync effect when it seems that it’s more likely to solve the original issue with some form of a writable derived

@trueadm
Copy link
Contributor

trueadm commented Aug 23, 2024

I don’t know if it is worth going down this rabbit hole of trying to add a sync effect when it seems that it’s more likely to solve the original issue with some form of a writable derived

Exactly. Deriveds are the correct solution for this problem, we just need a mutable version or some permutation of it in the future that deals with this use case.

@mimbrown
Copy link
Contributor Author

There's probably an alternative solution to this problem, but I doubt this is the only time a sync effect would be useful. Here's what the Vue docs say:

In rare cases, it might be necessary to trigger a watcher immediately when a reactive dependency changes, e.g. to invalidate a cache. This can be achieved using flush: 'sync'. However, this setting should be used with caution, as it can lead to problems with performance and data consistency if multiple properties are being updated at the same time.

So pretty much exactly what I'm proposing: "You might need this, we support it, use with caution."

@trueadm
Copy link
Contributor

trueadm commented Aug 23, 2024

There's probably an alternative solution to this problem, but I doubt this is the only time a sync effect would be useful. Here's what the Vue docs say:

In rare cases, it might be necessary to trigger a watcher immediately when a reactive dependency changes, e.g. to invalidate a cache. This can be achieved using flush: 'sync'. However, this setting should be used with caution, as it can lead to problems with performance and data consistency if multiple properties are being updated at the same time.

So pretty much exactly what I'm proposing: "You might need this, we support it, use with caution."

We have flushSync that serves the same purpose.

@Antonio-Bennett

This comment was marked as duplicate.

@mimbrown
Copy link
Contributor Author

@trueadm isn't flushSync a little overkill? Like I'm thinking, if you needed to implement something in userland, and it needs a sync effect (maybe possibly sometimes), then you have to always flush synchronously, which affects everything.

@trueadm
Copy link
Contributor

trueadm commented Sep 17, 2024

@mimbrown Yes, you need to flush everything. If you have a parent {#if} block, which is just an effect under the hood, and you have a sync effect inside that if effect, then you'd expect that in the case the if effect invalidates (it changes from true to false ) then the child sync effect should never run. However, because the if effect itself is not sync, that won't happen and the entire system will fall apart.

For these reasons, we're not going to introduce sync effects in to Svelte 5. I hope all the details in nuance here make sense as to why this isn't feasible too. :)

@trueadm trueadm closed this Sep 17, 2024
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.

3 participants