-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: make $effect.active()
true when updating deriveds
#11500
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
Changes from all commits
1f76e87
7987338
7e2740a
48ef3e9
4a556c9
f719ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'svelte': patch | ||
--- | ||
|
||
fix: make `$effect.active()` true when updating deriveds |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,20 +26,20 @@ import { | |
EFFECT_RAN, | ||
BLOCK_EFFECT, | ||
ROOT_EFFECT, | ||
EFFECT_TRANSPARENT | ||
EFFECT_TRANSPARENT, | ||
DERIVED, | ||
UNOWNED | ||
} from '../constants.js'; | ||
import { set } from './sources.js'; | ||
import { remove } from '../dom/reconciler.js'; | ||
import * as e from '../errors.js'; | ||
import { DEV } from 'esm-env'; | ||
|
||
/** | ||
* @param {import('#client').Effect | null} effect | ||
* @param {'$effect' | '$effect.pre' | '$inspect'} rune | ||
* @returns {asserts effect} | ||
*/ | ||
export function validate_effect(effect, rune) { | ||
if (effect === null) { | ||
export function validate_effect(rune) { | ||
if (current_effect === null && current_reaction === null) { | ||
e.effect_orphan(rune); | ||
} | ||
|
||
|
@@ -93,6 +93,18 @@ function create_effect(type, fn, sync) { | |
} | ||
|
||
if (current_reaction !== null && !is_root) { | ||
var flags = current_reaction.f; | ||
if ((flags & DERIVED) !== 0) { | ||
if ((flags & UNOWNED) !== 0) { | ||
e.effect_in_unowned_derived(); | ||
} | ||
// If we are inside a derived, then we also need to attach the | ||
// effect to the parent effect too. | ||
if (current_effect !== null) { | ||
push_effect(effect, current_effect); | ||
} | ||
} | ||
|
||
push_effect(effect, current_reaction); | ||
} | ||
|
||
|
@@ -118,20 +130,29 @@ function create_effect(type, fn, sync) { | |
* @returns {boolean} | ||
*/ | ||
export function effect_active() { | ||
return current_effect ? (current_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0 : false; | ||
if (current_reaction && (current_reaction.f & DERIVED) !== 0) { | ||
return (current_reaction.f & UNOWNED) === 0; | ||
} | ||
|
||
if (current_effect) { | ||
return (current_effect.f & (BRANCH_EFFECT | ROOT_EFFECT)) === 0; | ||
} | ||
|
||
return false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the order here seems slightly different to before. Before, if a derived was unowned but executed as part of an effect (i.e. current_effect was set) then it would be counted as active. Now it won't be anymore. Is that on purpose? If so, why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See https://github.com/sveltejs/svelte/pull/11500/files#r1594543348. I think it's important that child effects belong directly to the derived, and not to a surrounding effect, otherwise you can end up with some confusing inconsistencies |
||
} | ||
|
||
/** | ||
* Internal representation of `$effect(...)` | ||
* @param {() => void | (() => void)} fn | ||
*/ | ||
export function user_effect(fn) { | ||
validate_effect(current_effect, '$effect'); | ||
validate_effect('$effect'); | ||
|
||
// Non-nested `$effect(...)` in a component should be deferred | ||
// until the component is mounted | ||
const defer = | ||
current_effect.f & RENDER_EFFECT && | ||
current_effect !== null && | ||
(current_effect.f & RENDER_EFFECT) !== 0 && | ||
// TODO do we actually need this? removing them changes nothing | ||
current_component_context !== null && | ||
!current_component_context.m; | ||
|
@@ -150,7 +171,7 @@ export function user_effect(fn) { | |
* @returns {import('#client').Effect} | ||
*/ | ||
export function user_pre_effect(fn) { | ||
validate_effect(current_effect, '$effect.pre'); | ||
validate_effect('$effect.pre'); | ||
return render_effect(fn); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { flushSync } from 'svelte'; | ||
import { test } from '../../test'; | ||
|
||
export default test({ | ||
html: ` | ||
<button>toggle outer</button> | ||
<button>toggle inner</button> | ||
<button>reset</button> | ||
`, | ||
|
||
test({ assert, target }) { | ||
const [outer, inner, reset] = target.querySelectorAll('button'); | ||
|
||
flushSync(() => outer?.click()); | ||
flushSync(() => inner?.click()); | ||
|
||
assert.htmlEqual( | ||
target.innerHTML, | ||
` | ||
<button>toggle outer</button> | ||
<button>toggle inner</button> | ||
<button>reset</button> | ||
<p>v is true</p> | ||
` | ||
); | ||
|
||
flushSync(() => reset?.click()); | ||
flushSync(() => inner?.click()); | ||
flushSync(() => outer?.click()); | ||
|
||
assert.htmlEqual( | ||
target.innerHTML, | ||
` | ||
<button>toggle outer</button> | ||
<button>toggle inner</button> | ||
<button>reset</button> | ||
<p>v is true</p> | ||
` | ||
); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<script> | ||
let value = $state(false); | ||
const fn = () => { | ||
if ($effect.active()) { | ||
$effect(() => { | ||
value = true; | ||
}); | ||
} | ||
return value; | ||
}; | ||
|
||
let outer = $state(false); | ||
let inner = $state(false); | ||
let v = $derived(inner ? fn() : false); | ||
</script> | ||
|
||
<button onclick={() => outer = !outer}> | ||
toggle outer | ||
</button> | ||
|
||
<button onclick={() => inner = !inner}> | ||
toggle inner | ||
</button> | ||
|
||
<button onclick={() => outer = inner = value = false}> | ||
reset | ||
</button> | ||
|
||
{#if outer && v} | ||
<p>v is true</p> | ||
{/if} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be?
Then it goes to the next block to check for an active current effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we do, no.
As things stand today, if it did fall through to the current effect, and as a result
$effect.active()
returned true leading to a new effect being created, that new effect would be a child of the unowned derived. As such, it would never run — it would just sit there taking up memory until the derived was GC'd.So we'd need a corresponding change inside
create_effect
:With that, effects created inside unowned deriveds do run as long as the derived value is being read inside an effect, as in this somewhat contrived example.
But there's a catch. If we read the derived outside an effect, the child effect isn't created because
$effect.active()
isfalse
. Meanwhile, it updates to the latest value. If we then read it inside an effect, the value is already up to date, so the function doesn't re-run, and the child effect still isn't created. If you read the derived value inside an effect first, the child effect is created. This feels like a sufficiently confusing difference in behaviour that we'd be much better off enforcing that unowned deriveds can't contain effects.Interestingly: in the demo above (on this branch and also with the changes above), the effect inside the owned derived is created but doesn't run if the derived is read inside an effect. But it does run if it's read outside an effect (try clicking 'read values' followed by 'increment'). That feels like a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed that change to an
effect-active-derived-allow-unowned
branch for posterity