ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/reactivity/map.js (JavaScript)

Code before:

		// If the value is invalid then the native exception will fire here
		if (DEV) new Map(value);

		if (value) {
			var sources = this.#sources;

			for (var [key, v] of value) {
				sources.set(key, source(Symbol()));
				super.set(key, v);
			}

			this.#size.v = sources.size;
		}
	}

	#increment_version() {
		set(this.#version, this.#version.v + 1);
	}

	/** @param {K} key */
	has(key) {
		var sources = this.#sources;
		var s = sources.get(key);

		if (s === undefined) {
			// If we're working with a non-primitive then we can generate a signal for it
			if (typeof key !== 'object' || key === null) {
				s = source(UNINITIALIZED);
				sources.set(key, s);
			} else {
				// We should always track the version in case
				// the Set ever gets this value in the future.
				get(this.#version);
				return false;
			}
		}

		get(s);
		return super.has(key);
	}

	/**
	 * @param {(value: V, key: K, map: Map<K, V>) => void} callbackfn
	 * @param {any} [this_arg]
	 */
	forEach(callbackfn, this_arg) {
		get(this.#version);

		return super.forEach(callbackfn, this_arg);
	}

Review comment (performance):
This still has the same problem as #11504 and #11287. We need a way to remove these signals when we are no longer listening to them. Otherwise it would create a memory leak (see https://github.com/sveltejs/svelte/pull/11504#issuecomment-2129529254)

Code after:

		// If the value is invalid then the native exception will fire here
		if (DEV) new Map(value);

		if (value) {
			for (var [key, v] of value) {
				super.set(key, v);
			}
			this.#size.v = super.size;
		}
	}

	#increment_version() {
		set(this.#version, this.#version.v + 1);
	}

	/** @param {K} key */
	has(key) {
		var sources = this.#sources;
		var s = sources.get(key);

		if (s === undefined) {
			var ret = super.get(key);
			if (ret !== undefined) {
				s = source(Symbol());
				sources.set(key, s);
			} else {
				// We should always track the version in case
				// the Set ever gets this value in the future.
				get(this.#version);
				return false;
			}
		}

		get(s);
		return true;
	}

	/**
	 * @param {(value: V, key: K, map: Map<K, V>) => void} callbackfn
	 * @param {any} [this_arg]
	 */
	forEach(callbackfn, this_arg) {
		get(this.#version);

		return super.forEach(callbackfn, this_arg);
	}

	/** @param {K} key */
	get(key) {
		var sources = this.#sources;
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/effects.js (JavaScript)

Code before:

/**
 * @param {import("#client").Effect} effect
 */
export function execute_effect_teardown(effect) {
	var teardown = effect.teardown;
	if (teardown !== null) {
		const previously_destroying_effect = is_destroying_effect;
		const previous_untracking = current_untracking;
		set_is_destroying_effect(true);
		set_untracking(true);
		try {
			teardown.call(null);
		} finally {
			set_is_destroying_effect(previously_destroying_effect);
			set_untracking(previous_untracking);
		}
	}
}

/**
 * @param {import('#client').Effect} effect
 * @param {boolean} skip_remove_dom
 * @returns {void}
 */
export function destroy_effect(effect, skip_remove_dom) {
	var dom = effect.dom;

	if (dom !== null && !skip_remove_dom) {
		remove(dom);
	}

	destroy_effect_children(effect, skip_remove_dom);
	remove_reactions(effect, 0);
	set_signal_status(effect, DESTROYED);

	if (effect.transitions) {
		for (const transition of effect.transitions) {
			transition.stop();
		}
	}

	execute_effect_teardown(effect);

	var parent = effect.parent;

	// If the parent doesn't have any children, then skip this work altogether
	if (parent !== null && (effect.f & BRANCH_EFFECT) !== 0 && parent.first !== null) {
		var previous = effect.prev;
		var next = effect.next;
		if (previous !== null) {

Review comment (suggestion):
Should the argument (here and the other function) be `remove_dom` instead, i.e. the inverse? Reads a bit better

Code after:

/**
 * @param {import("#client").Effect} effect
 */
export function execute_effect_teardown(effect) {
	var teardown = effect.teardown;
	if (teardown !== null) {
		const previously_destroying_effect = is_destroying_effect;
		const previous_untracking = current_untracking;
		set_is_destroying_effect(true);
		set_untracking(true);
		try {
			teardown.call(null);
		} finally {
			set_is_destroying_effect(previously_destroying_effect);
			set_untracking(previous_untracking);
		}
	}
}

/**
 * @param {import('#client').Effect} effect
 * @param {boolean} [remove_dom]
 * @returns {void}
 */
export function destroy_effect(effect, remove_dom = true) {
	var dom = effect.dom;

	if (dom !== null && remove_dom) {
		remove(dom);
	}

	destroy_effect_children(effect, remove_dom);
	remove_reactions(effect, 0);
	set_signal_status(effect, DESTROYED);

	if (effect.transitions) {
		for (const transition of effect.transitions) {
			transition.stop();
		}
	}

	execute_effect_teardown(effect);

	var parent = effect.parent;

	// If the parent doesn't have any children, then skip this work altogether
	if (parent !== null && (effect.f & BRANCH_EFFECT) !== 0 && parent.first !== null) {
		var previous = effect.prev;
		var next = effect.next;
		if (previous !== null) {
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dev/hmr.js (JavaScript)

Code before:
import { block, branch, destroy_effect } from '../reactivity/effects.js';
import { set_should_intro } from '../render.js';
import { get } from '../runtime.js';
import { check_target } from './legacy.js';

/**
 * @template {(anchor: Comment, props: any) => any} Component
 * @param {import("#client").Source<Component>} source
 */
export function hmr(source) {
	/**
	 * @param {Comment} anchor
	 * @param {any} props
	 */
	return function (anchor, props) {
		let instance = {};

		/** @type {import("#client").Effect} */
		let effect;

		block(() => {
			const component = get(source);

			// @ts-expect-error
			check_target(new.target && component);

			if (effect) {
				// @ts-ignore
				for (var k in instance) delete instance[k];
				destroy_effect(effect);
			}

			effect = branch(() => {
				set_should_intro(false);
				// preserve getters/setters
				Object.defineProperties(
					instance,
					Object.getOwnPropertyDescriptors(component(anchor, props))
				);
				set_should_intro(true);
			});
		});

		return instance;
	};
}


Review comment (bug):
this does the check and then fail even when the compiler option is set, which shouldn't happen in that case

Code after:
import { block, branch, destroy_effect } from '../reactivity/effects.js';
import { set_should_intro } from '../render.js';
import { get } from '../runtime.js';
import { check_target } from './legacy.js';

/**
 * @template {(anchor: Comment, props: any) => any} Component
 * @param {import("#client").Source<Component>} source
 */
export function hmr(source) {
	/**
	 * @param {Comment} anchor
	 * @param {any} props
	 */
	return function (anchor, props) {
		let instance = {};

		/** @type {import("#client").Effect} */
		let effect;

		block(() => {
			const component = get(source);

			if (effect) {
				// @ts-ignore
				for (var k in instance) delete instance[k];
				destroy_effect(effect);
			}

			effect = branch(() => {
				set_should_intro(false);
				// preserve getters/setters
				Object.defineProperties(
					instance,
					Object.getOwnPropertyDescriptors(
						// @ts-expect-error
						new.target ? new component(anchor, props) : component(anchor, props)
					)
				);
				set_should_intro(true);
			});
		});

		return instance;
	};
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/server/types.d.ts (TypeScript)

Code before:
	p: null | Component;
	/** context */
	c: null | Map<unknown, unknown>;
	/** ondestroy */
	d: null | Array<() => void>;
	/**
	 * dev mode only: the component function
	 */
	function?: any;
}

export interface Payload {
	out: string;
	anchor: number;
	head: {
		title: string;
		out: string;
		anchor: number;
	};
}

export interface RenderOutput {
	head: string;
	/** @deprecated */
	html: string;
	body: string;
}


Review comment (suggestion):
```suggestion
	/** HTML that goes into the `<head>` */
	head: string;
	/** @deprecated use `body` instead */
	html: string;
	/** HTML that goes somewhere into the `<body>` */
	body: string;
```

Code after:
	p: null | Component;
	/** context */
	c: null | Map<unknown, unknown>;
	/** ondestroy */
	d: null | Array<() => void>;
	/**
	 * dev mode only: the component function
	 */
	function?: any;
}

export interface Payload {
	out: string;
	anchor: number;
	head: {
		title: string;
		out: string;
		anchor: number;
	};
}

export interface RenderOutput {
	/** HTML that goes into the `<head>` */
	head: string;
	/** @deprecated use `body` instead */
	html: string;
	/** HTML that goes somewhere into the `<body>` */
	body: string;
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/render.js (JavaScript)

Code before:

/** @type {Set<string>} */
export const all_registered_events = new Set();

/** @type {Set<(events: Array<string>) => void>} */
export const root_event_handles = new Set();

/**
 * This is normally true — block effects should run their intro transitions —
 * but is false during hydration and mounting (unless `options.intro` is `true`)
 * and when creating the children of a `<svelte:element>` that just changed tag
 */
export let should_intro = true;

/** @param {boolean} value */
export function set_should_intro(value) {
	should_intro = value;
}

/**
 * @param {Element} text
 * @param {string} value
 * @returns {void}
 */
export function set_text(text, value) {
	text.nodeValue = value;
}

/**
 * @param {Comment} anchor
 * @param {void | ((anchor: Comment, slot_props: Record<string, unknown>) => void)} slot_fn
 * @param {Record<string, unknown>} slot_props
 * @param {null | ((anchor: Comment) => void)} fallback_fn
 */
export function slot(anchor, slot_fn, slot_props, fallback_fn) {
	if (slot_fn === undefined) {
		if (fallback_fn !== null) {
			fallback_fn(anchor);
		}
	} else {
		slot_fn(anchor, slot_props);
	}
}

/**
 * @param {unknown} value
 * @returns {string}
 */
export function stringify(value) {
	return typeof value === 'string' ? value : value == null ? '' : value + '';
}

Review comment (performance):
Are you removing skipping during hydration because reading `nodeValue` to eventually skip if eventually more expensive than just set the value?

Code after:

/** @type {Set<string>} */
export const all_registered_events = new Set();

/** @type {Set<(events: Array<string>) => void>} */
export const root_event_handles = new Set();

/**
 * This is normally true — block effects should run their intro transitions —
 * but is false during hydration and mounting (unless `options.intro` is `true`)
 * and when creating the children of a `<svelte:element>` that just changed tag
 */
export let should_intro = true;

/** @param {boolean} value */
export function set_should_intro(value) {
	should_intro = value;
}

/**
 * @param {Element} text
 * @param {string} value
 * @returns {void}
 */
export function set_text(text, value) {
	// @ts-expect-error
	const prev = (text.__t ??= text.nodeValue);

	if (prev !== value) {
		// @ts-expect-error
		text.nodeValue = text.__t = value;
	}
}

/**
 * @param {Comment} anchor
 * @param {void | ((anchor: Comment, slot_props: Record<string, unknown>) => void)} slot_fn
 * @param {Record<string, unknown>} slot_props
 * @param {null | ((anchor: Comment) => void)} fallback_fn
 */
export function slot(anchor, slot_fn, slot_props, fallback_fn) {
	if (slot_fn === undefined) {
		if (fallback_fn !== null) {
			fallback_fn(anchor);
		}
	} else {
		slot_fn(anchor, slot_props);
	}
}

/**
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/effects.js (JavaScript)

Code before:
	/** @type {import('#client').Effect} */
	var effect = {
		ctx: current_component_context,
		deps: null,
		dom: null,
		f: type | DIRTY,
		first: null,
		fn,
		last: null,
		next: null,
		parent: is_root ? null : current_effect,
		prev: null,
		teardown: null,
		transitions: null
	};

	if (DEV) {
		effect.component_function = dev_current_component_function;
	}

	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);
	}

	if (sync) {
		var previously_flushing_effect = is_flushing_effect;

		try {
			set_is_flushing_effect(true);
			execute_effect(effect);
			effect.f |= EFFECT_RAN;
		} finally {
			set_is_flushing_effect(previously_flushing_effect);
		}
	} else if (fn !== null) {
		schedule_effect(effect);
	}

	return effect;

Review comment (suggestion):
Is it safe to execute the effect before linking it to its parent? If we have error boundaries in the future, I imagine we have to walk up the effect tree, but it's not constructed yet then.

Code after:
	/** @type {import('#client').Effect} */
	var effect = {
		ctx: current_component_context,
		deps: null,
		dom: null,
		f: type | DIRTY,
		first: null,
		fn,
		last: null,
		next: null,
		parent: is_root ? null : current_effect,
		prev: null,
		teardown: null,
		transitions: null
	};

	if (DEV) {
		effect.component_function = dev_current_component_function;
	}

	if (sync) {
		var previously_flushing_effect = is_flushing_effect;

		try {
			set_is_flushing_effect(true);
			execute_effect(effect);
			effect.f |= EFFECT_RAN;
		} finally {
			set_is_flushing_effect(previously_flushing_effect);
		}
	} else if (fn !== null) {
		schedule_effect(effect);
	}

	// if an effect has no dependencies, no DOM and no teardown function,
	// don't bother adding it to the effect tree
	var inert =
		sync &&
		effect.deps === null &&
		effect.first === null &&
		effect.dom === null &&
		effect.teardown === null;

	if (!inert && 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.
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/elements/bindings/input.js (JavaScript)

Code before:
			// @ts-ignore
			input.checked = is(input.__value, value);
		}
	});

	teardown(() => {
		var index = binding_group.indexOf(input);

		if (index !== -1) {
			binding_group.splice(index, 1);
		}
	});

	queue_micro_task(() => {
		// necessary to maintain binding group order in all insertion scenarios. TODO optimise
		binding_group.sort((a, b) => (a.compareDocumentPosition(b) === 4 ? -1 : 1));
		if (hydration_mismatch) {
			var value;
			if (is_checkbox) {
				value = get_binding_group_value(binding_group, value, input.checked);
			} else {
				var hydration_input = binding_group.find((input) => input.checked);
				if (hydration_input !== undefined) {
					// @ts-ignore
					value = hydration_input.__value;
				}
			}
			update(value);
		}
	});
}

/**
 * @param {HTMLInputElement} input
 * @param {() => unknown} get_value
 * @param {(value: unknown) => void} update
 * @returns {void}
 */
export function bind_checked(input, get_value, update) {
	listen_to_event_and_reset_event(input, 'change', () => {
		var value = input.checked;
		update(value);
	});

	if (get_value() == undefined) {
		update(false);
	}

	render_effect(() => {
		var value = get_value();
		input.checked = Boolean(value);

Review comment (suggestion):
We can code golf this a bit
```suggestion
				// @ts-ignore
				value = hydration_input?.__value;
```

Code after:
			// @ts-ignore
			input.checked = get_proxied_value(value).includes(get_proxied_value(input.__value));
		} else {
			// @ts-ignore
			input.checked = is(input.__value, value);
		}
	});

	teardown(() => {
		var index = binding_group.indexOf(input);

		if (index !== -1) {
			binding_group.splice(index, 1);
		}
	});

	queue_micro_task(() => {
		// necessary to maintain binding group order in all insertion scenarios. TODO optimise
		binding_group.sort((a, b) => (a.compareDocumentPosition(b) === 4 ? -1 : 1));

		if (hydration_mismatch) {
			var value;

			if (is_checkbox) {
				value = get_binding_group_value(binding_group, value, input.checked);
			} else {
				var hydration_input = binding_group.find((input) => input.checked);
				// @ts-ignore
				value = hydration_input?.__value;
			}

			update(value);
		}
	});
}

/**
 * @param {HTMLInputElement} input
 * @param {() => unknown} get_value
 * @param {(value: unknown) => void} update
 * @returns {void}
 */
export function bind_checked(input, get_value, update) {
	listen_to_event_and_reset_event(input, 'change', () => {
		var value = input.checked;
		update(value);
	});

	if (get_value() == undefined) {
		update(false);
	}
Repository: sveltejs/svelte
File: packages/svelte/tests/runtime-runes/samples/await-resolve/main.svelte (Svelte)

Code before:
<script>
	import Pending from "./Pending.svelte"
	import Then from './Then.svelte';

	const promise_a = Promise.resolve('a')
	const promise_b = Promise.resolve('b')

	let current_promise = $state(promise_a);
</script>

{#await current_promise}
	<Pending />
{:then value}
	<Then {value} />
{/await}

<button onclick={()=>{current_promise = promise_a}}>Show Promise A</button>
<button onclick={()=>{current_promise = promise_b}}>Show Promise B</button>


Review comment (suggestion):
This is what I did to ensure that it doesn't even render - also simplifies the test because you don't need the additional two components anymore
```suggestion
	{console.log('pending')}
{:then value}
	{console.log(value)}
```

Code after:
<script>
	const a = Promise.resolve('a');
	const b = Promise.resolve('b');

	let promise = $state(a);
</script>

{#await promise}
	{console.log('rendering pending block')}
	<p>pending</p>
{:then value}
	{console.log('rendering then block')}
	<p>then {value}</p>
{/await}

<button onclick={() => (promise = a)}>Show Promise A</button>
<button onclick={() => (promise = b)}>Show Promise B</button>
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/dom/blocks/await.js (JavaScript)

Code before:
				var parent_effect = current_effect;
				var parent_reaction = current_reaction;
				var show_pending = () => {
					if (resolved) return;
					pending = true;
					if (pending_effect && (pending_effect.f & INERT) === 0) {
						destroy_effect(pending_effect);
					}
					var previous_effect = current_effect;
					var previous_reaction = current_reaction;
					try {
						set_current_effect(parent_effect);
						set_current_reaction(parent_reaction);
						pending_effect = branch(() => pending_fn(anchor));
						if (then_effect) pause_effect(then_effect);
					} finally {
						set_current_effect(previous_effect);
						set_current_reaction(previous_reaction);
					}
				};

				if (hydrating) {
					show_pending();
				} else {
					pending = false;
					// Wait a microtask before checking if we should the pending state as
					// the promise might have resolved by the next microtask.
					queue_micro_task(show_pending);
				}
			} else if (then_effect) {
				pending = true;
				pause_effect(then_effect);
			}

			if (catch_effect) pause_effect(catch_effect);
		} else {
			if (pending_effect) pause_effect(pending_effect);
			if (catch_effect) pause_effect(catch_effect);

			if (then_fn) {
				if (then_effect) {
					destroy_effect(then_effect);
				}

				set(input_source, input);
				then_effect = branch(() => then_fn(anchor, /** @type {V} */ (input_source)));
			}
		}

		// Inert effects are proactively detached from the effect tree. Returning a noop
		// teardown function is an easy way to ensure that this is not discarded

Review comment (suggestion):
```suggestion
					// Wait a microtask before checking if we should show the pending state as
```

Code after:

			promise.then(
				(value) => {
					if (promise !== input) return;
					set(input_source, value);
					update(THEN, true);
				},
				(error) => {
					if (promise !== input) return;
					set(error_source, error);
					update(CATCH, true);
				}
			);

			if (hydrating) {
				if (pending_fn) {
					pending_effect = branch(() => pending_fn(anchor));
				}
			} else {
				// Wait a microtask before checking if we should show the pending state as
				// the promise might have resolved by the next microtask.
				queue_micro_task(() => {
					if (!resolved) update(PENDING, true);
				});
			}
		} else {
			set(input_source, input);
			update(THEN, false);
		}

		// Inert effects are proactively detached from the effect tree. Returning a noop
		// teardown function is an easy way to ensure that this is not discarded
		return noop;
	});
}
Repository: sveltejs/svelte
File: packages/svelte/src/reactivity/set.js (JavaScript)

Code before:

	/** @param {T} value */
	delete(value) {
		var deleted = super.delete(value);
		var sources = this.#sources;
		var s = sources.get(value);

		if (s !== undefined) {
			sources.delete(value);
			set(s, false);
		}

		if (deleted) {
			set(this.#size, super.size);
			increment(this.#version);
		}

		return deleted;
	}

	clear() {
		var size = super.size;
		// Clear first, so we get nice console.log outputs with $inspect
		super.clear();

		if (size !== 0) {
			var sources = this.#sources;

			for (var s of sources.values()) {
				set(s, false);
			}

			sources.clear();
			set(this.#size, 0);
			increment(this.#version);
		}
	}

	keys() {
		return this.values();
	}

	values() {
		get(this.#version);
		return super.values();
	}

	entries() {
		get(this.#version);
		return super.entries();
	}

Review comment (suggestion):
should this go inside the `if` block? no point clearing if size is already 0

Code after:

	/** @param {T} value */
	delete(value) {
		var deleted = super.delete(value);
		var sources = this.#sources;
		var s = sources.get(value);

		if (s !== undefined) {
			sources.delete(value);
			set(s, false);
		}

		if (deleted) {
			set(this.#size, super.size);
			increment(this.#version);
		}

		return deleted;
	}

	clear() {
		if (super.size === 0) {
			return;
		}
		// Clear first, so we get nice console.log outputs with $inspect
		super.clear();
		var sources = this.#sources;

		for (var s of sources.values()) {
			set(s, false);
		}

		sources.clear();
		set(this.#size, 0);
		increment(this.#version);
	}

	keys() {
		return this.values();
	}

	values() {
		get(this.#version);
		return super.values();
	}

	entries() {
		get(this.#version);
		return super.entries();
	}