Skip to content

Commit

Permalink
fix: ensure spread events are always added
Browse files Browse the repository at this point in the history
In edge cases it may happen that set_attributes is re-run before the effect is executed. In that case the render effect which initiates this re-run will destroy the inner effect and it will never run. But because next and prev may have the same keys, the event would not get added again and it would get lost. We prevent this by using a root effect.
The added test case doesn't fail for some reason without this fix, but it does fail when you test it out manually, so I still added it.
Found through #10359 (comment)
  • Loading branch information
dummdidumm committed May 10, 2024
1 parent 59f4feb commit 45b83a7
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 5 deletions.
5 changes: 5 additions & 0 deletions .changeset/two-brooms-fail.md
@@ -0,0 +1,5 @@
---
"svelte": patch
---

fix: ensure spread events are added even when rerunning spread immediately
24 changes: 19 additions & 5 deletions packages/svelte/src/internal/client/dom/elements/attributes.js
Expand Up @@ -4,8 +4,7 @@ import { get_descriptors, get_prototype_of, map_get, map_set } from '../../utils
import { AttributeAliases, DelegatedEvents, namespace_svg } from '../../../../constants.js';
import { delegate } from './events.js';
import { autofocus } from './misc.js';
import { effect } from '../../reactivity/effects.js';
import { run } from '../../../shared/utils.js';
import { effect, effect_root } from '../../reactivity/effects.js';
import * as w from '../../warnings.js';

/**
Expand Down Expand Up @@ -112,7 +111,7 @@ export function set_attributes(element, prev, next, lowercase_attributes, css_ha

// @ts-expect-error
var attributes = /** @type {Record<string, unknown>} **/ (element.__attributes ??= {});
/** @type {Array<() => void>} */
/** @type {Array<[string, any, () => void]>} */
var events = [];

for (key in next) {
Expand Down Expand Up @@ -145,7 +144,7 @@ export function set_attributes(element, prev, next, lowercase_attributes, css_ha
if (value != null) {
if (!delegated) {
if (!prev) {
events.push(() => element.addEventListener(event_name, value, opts));
events.push([key, value, () => element.addEventListener(event_name, value, opts)]);
} else {
element.addEventListener(event_name, value, opts);
}
Expand Down Expand Up @@ -193,7 +192,22 @@ export function set_attributes(element, prev, next, lowercase_attributes, css_ha
// On the first run, ensure that events are added after bindings so
// that their listeners fire after the binding listeners
if (!prev) {
effect(() => events.forEach(run));
// In edge cases it may happen that set_attributes is re-run before the
// effect is executed. In that case the render effect which initiates this
// re-run will destroy the inner effect and it will never run. But because
// next and prev may have the same keys, the event would not get added again
// and it would get lost. We prevent this by using a root effect.
const destroy_root = effect_root(() => {
effect(() => {
if (!element.isConnected) return;
for (const [key, value, evt] of events) {
if (next[key] === value) {
evt();
}
}
destroy_root();
});
});
}

return next;
Expand Down
@@ -0,0 +1,15 @@
import { test, ok } from '../../test';

export default test({
mode: ['client'],

async test({ assert, logs, target }) {
const input = target.querySelector('input');
ok(input);

input.value = 'foo';
await input.dispatchEvent(new Event('input'));

assert.deepEqual(logs, ['hi']);
}
});
@@ -0,0 +1,7 @@
<script>
let rest = $state(undefined);
</script>

<input {...rest} oninput={() => console.log('hi')}>
<!-- after input and inside template so that attribute spread reruns immediately -->
{!rest ? (rest = {}) : false}

0 comments on commit 45b83a7

Please sign in to comment.