ronantakizawa/codereview-bench huggingface dataset:
Repository: sveltejs/svelte
File: packages/svelte/src/store/index-client.js (JavaScript)

Code before:
 */
/**
 * @template V
 * @overload
 * @param {() => V} get
 * @returns {Readable<V>}
 */
/**
 * Create a store from a function that returns state, and (to make a writable store), an
 * optional second function that sets state.
 *
 * ```ts
 * import { toStore } from 'svelte/store';
 *
 * let count = $state(0);
 *
 * const store = toStore(() => count, (v) => (count = v));
 * ```
 * @template V
 * @param {() => V} get
 * @param {(v: V) => void} [set]
 * @returns {Writable<V> | Readable<V>}
 */
export function toStore(get, set) {
	let init_value = get();
	const store = writable(get(), (set) => {
		// If the value has changed before we call subscribe, then
		// we need to treat the value as already having run
		let ran = init_value !== get();

		// TODO do we need a different implementation on the server?
		const teardown = effect_root(() => {
			render_effect(() => {
				const value = get();
				if (ran) set(value);
			});
		});

		ran = true;

		return teardown;
	});

	if (set) {
		return {
			set,
			update: (fn) => set(fn(get())),
			subscribe: store.subscribe
		};
	}


Review comment (suggestion):
```suggestion
	const store = writable(init_value, (set) => {
```

Code after:
 */
/**
 * @template V
 * @overload
 * @param {() => V} get
 * @returns {Readable<V>}
 */
/**
 * Create a store from a function that returns state, and (to make a writable store), an
 * optional second function that sets state.
 *
 * ```ts
 * import { toStore } from 'svelte/store';
 *
 * let count = $state(0);
 *
 * const store = toStore(() => count, (v) => (count = v));
 * ```
 * @template V
 * @param {() => V} get
 * @param {(v: V) => void} [set]
 * @returns {Writable<V> | Readable<V>}
 */
export function toStore(get, set) {
	let init_value = get();
	const store = writable(init_value, (set) => {
		// If the value has changed before we call subscribe, then
		// we need to treat the value as already having run
		let ran = init_value !== get();

		// TODO do we need a different implementation on the server?
		const teardown = effect_root(() => {
			render_effect(() => {
				const value = get();
				if (ran) set(value);
			});
		});

		ran = true;

		return teardown;
	});

	if (set) {
		return {
			set,
			update: (fn) => set(fn(get())),
			subscribe: store.subscribe
		};
	}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/BindDirective.js (JavaScript)

Code before:
					'$.bind_group',
					node.metadata.binding_group_name,
					b.array(indexes),
					context.state.node,
					group_getter,
					set ?? get
				);
				break;
			}

			default:
				throw new Error('unknown binding ' + node.name);
		}
	}

	// Bindings need to happen after attribute updates, therefore after the render effect, and in order with events/actions.
	// bind:this is a special case as it's one-way and could influence the render effect.
	if (node.name === 'this') {
		context.state.init.push(b.stmt(call));
	} else {
		const has_action_directive =
			parent.type === 'RegularElement' && parent.attributes.find((a) => a.type === 'UseDirective');

		context.state.after_update.push(
			b.stmt(has_action_directive ? b.call('$.effect', b.thunk(call)) : call)
		);
	}
}


Review comment (suggestion):
no big deal but seemed no longer necessary to check again.

```suggestion
				b.stmt(b.call('$.effect', b.thunk(call)))
```

Code after:
					'$.bind_group',
					node.metadata.binding_group_name,
					b.array(indexes),
					context.state.node,
					group_getter,
					set ?? get
				);
				break;
			}

			default:
				throw new Error('unknown binding ' + node.name);
		}
	}

	// Bindings need to happen after attribute updates, therefore after the render effect, and in order with events/actions.
	// bind:this is a special case as it's one-way and could influence the render effect.
	if (node.name === 'this') {
		context.state.init.push(b.stmt(call));
	} else {
		const has_action_directive =
			parent.type === 'RegularElement' && parent.attributes.find((a) => a.type === 'UseDirective');

		if (has_action_directive) {
			context.state.init.push(
				b.stmt(has_action_directive ? b.call('$.effect', b.thunk(call)) : call)
			);
		} else {
			context.state.after_update.push(b.stmt(call));
		}
	}
}
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/runtime.js (JavaScript)

Code before:
	var signal;

	try {
		fn();
	} finally {
		is_signals_recorded = previous_is_signals_recorded;
		if (is_signals_recorded) {
			for (signal of captured_signals) {
				previous_captured_signals.add(signal);
			}
		}
		captured_signals = previous_captured_signals;
	}

	return captured;
}

/**
 * Invokes a function and captures all signals that are read during the invocation,
 * then invalidates them.
 * @param {() => any} fn
 */
export function invalidate_inner_signals(fn) {
	var captured = capture_signals(() => untrack(fn));

	for (signal of captured) {
		// Go one level up because derived signals created as part of props in legacy mode
		if ((signal.f & LEGACY_DERIVED_PROP) !== 0) {
			for (const dep of /** @type {Derived} */ (signal).deps || []) {
				if ((dep.f & DERIVED) === 0) {
					mutate(dep, null /* doesnt matter */);
				}
			}
		} else {
			mutate(signal, null /* doesnt matter */);
		}
	}
}

/**
 * When used inside a [`$derived`](https://svelte.dev/docs/svelte/$derived) or [`$effect`](https://svelte.dev/docs/svelte/$effect),
 * any state read inside `fn` will not be treated as a dependency.
 *
 * ```ts
 * $effect(() => {
 *   // this will run when `data` changes, but not when `time` changes
 *   save(data, {
 *     timestamp: untrack(() => time)
 *   });
 * });
 * ```

Review comment (suggestion):
```suggestion
	for (var signal of captured) {
```

Code after:
		untrack(fn);
		if (previous_captured_signals !== null) {
			for (signal of captured_signals) {
				previous_captured_signals.add(signal);
			}
		}
	} finally {
		captured_signals = previous_captured_signals;
	}

	return captured;
}

/**
 * Invokes a function and captures all signals that are read during the invocation,
 * then invalidates them.
 * @param {() => any} fn
 */
export function invalidate_inner_signals(fn) {
	var captured = capture_signals(() => untrack(fn));

	for (var signal of captured) {
		// Go one level up because derived signals created as part of props in legacy mode
		if ((signal.f & LEGACY_DERIVED_PROP) !== 0) {
			for (const dep of /** @type {Derived} */ (signal).deps || []) {
				if ((dep.f & DERIVED) === 0) {
					mutate(dep, null /* doesnt matter */);
				}
			}
		} else {
			mutate(signal, null /* doesnt matter */);
		}
	}
}

/**
 * When used inside a [`$derived`](https://svelte.dev/docs/svelte/$derived) or [`$effect`](https://svelte.dev/docs/svelte/$effect),
 * any state read inside `fn` will not be treated as a dependency.
 *
 * ```ts
 * $effect(() => {
 *   // this will run when `data` changes, but not when `time` changes
 *   save(data, {
 *     timestamp: untrack(() => time)
 *   });
 * });
 * ```
 * @template T
 * @param {() => T} fn
 * @returns {T}
 */
Repository: sveltejs/svelte
File: packages/svelte/src/internal/client/reactivity/effects.js (JavaScript)

Code before:
 * @param {number} [line]
 * @param {number} [column]
 */
export function legacy_pre_effect(deps, fn, line, column) {
	var context = /** @type {ComponentContextLegacy} */ (component_context);

	/** @type {{ effect: null | Effect, ran: boolean }} */
	var token = { effect: null, ran: false };
	context.l.r1.push(token);

	if (DEV && line !== undefined) {
		var location = get_location(line, column);
		var explicit_deps = capture_signals(deps);
	}

	token.effect = render_effect(() => {
		deps();

		// If this legacy pre effect has already run before the end of the reset, then
		// bail out to emulate the same behavior.
		if (token.ran) return;

		token.ran = true;
		set(context.l.r2, true);

		if (location) {
			var implicit_deps = capture_signals(() => untrack(fn));

			for (const signal of implicit_deps) {
				if (!explicit_deps.has(signal)) {
					w.reactive_declaration_non_reactive_property(/** @type {string} */ (location));
				}
			}
		}
	});
}

export function legacy_pre_effect_reset() {
	var context = /** @type {ComponentContextLegacy} */ (component_context);

	render_effect(() => {
		if (!get(context.l.r2)) return;

		// Run dirty `$:` statements
		for (var token of context.l.r1) {
			var effect = token.effect;

			// If the effect is CLEAN, then make it MAYBE_DIRTY. This ensures we traverse through
			// the effects dependencies and correctly ensure each dependency is up-to-date.
			if ((effect.f & CLEAN) !== 0) {
				set_signal_status(effect, MAYBE_DIRTY);

Review comment (suggestion):
better safe than sorry

```suggestion
		if (DEV && location) {
```

Code after:
 * @param {number} [line]
 * @param {number} [column]
 */
export function legacy_pre_effect(deps, fn, line, column) {
	var context = /** @type {ComponentContextLegacy} */ (component_context);

	/** @type {{ effect: null | Effect, ran: boolean }} */
	var token = { effect: null, ran: false };
	context.l.r1.push(token);

	if (DEV && line !== undefined) {
		var location = get_location(line, column);
		var explicit_deps = capture_signals(deps);
	}

	token.effect = render_effect(() => {
		deps();

		// If this legacy pre effect has already run before the end of the reset, then
		// bail out to emulate the same behavior.
		if (token.ran) return;

		token.ran = true;
		set(context.l.r2, true);

		if (DEV && location) {
			var implicit_deps = capture_signals(() => untrack(fn));

			for (var signal of implicit_deps) {
				if (!explicit_deps.has(signal)) {
					w.reactive_declaration_non_reactive_property(/** @type {string} */ (location));
				}
			}
		} else {
			untrack(fn);
		}
	});
}

export function legacy_pre_effect_reset() {
	var context = /** @type {ComponentContextLegacy} */ (component_context);

	render_effect(() => {
		if (!get(context.l.r2)) return;

		// Run dirty `$:` statements
		for (var token of context.l.r1) {
			var effect = token.effect;

			// If the effect is CLEAN, then make it MAYBE_DIRTY. This ensures we traverse through
			// the effects dependencies and correctly ensure each dependency is up-to-date.
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js (JavaScript)

Code before:
	if (node.type !== 'RegularElement') return false;
	if (node.fragment.metadata.dynamic) return false;

	for (const attribute of node.attributes) {
		if (attribute.type !== 'Attribute') {
			return false;
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.value !== true && !is_text_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// an img with loading lazy is not static but needs the `img` to be
		// generated to be passed to `handle_lazy_img`
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (node.name.includes('-')) {
			return false; // we're setting all attributes on custom elements through properties
		}
	}

	return true;
}


Review comment (performance):
I couldn't understand this comment without reading through the code as it assumes some familiarity with why `handle_lazy_img` is needed. How about something like this instead?
```suggestion
		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
```

Code after:
	if (node.type !== 'RegularElement') return false;
	if (node.fragment.metadata.dynamic) return false;

	for (const attribute of node.attributes) {
		if (attribute.type !== 'Attribute') {
			return false;
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.value !== true && !is_text_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (node.name.includes('-')) {
			return false; // we're setting all attributes on custom elements through properties
		}
	}

	return true;
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js (JavaScript)

Code before:
		skipped -= 1;
		state.init.push(b.stmt(b.call('$.next', skipped !== 1 && b.literal(skipped))));
	}
}

/**
 * @param {SvelteNode} node
 * @param {ComponentContext["state"]["scope"]} scope
 */
function is_static_element(node, scope) {
	if (node.type !== 'RegularElement') return false;
	if (node.fragment.metadata.dynamic) return false;

	for (const attribute of node.attributes) {
		if (attribute.type !== 'Attribute') {
			return false;
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.value !== true && !is_text_attribute(attribute)) {
			// in this situation we are already inlining the binding in the template
			// so even if it's not a text attribute we can consider the element static
			if (
				typeof attribute.value !== 'boolean' &&
				!Array.isArray(attribute.value) &&
				attribute.value.expression.type === 'Identifier'
			) {
				const binding = scope.get(attribute.value.expression.name);
				if (binding) {
					return can_inline_variable(binding);
				}
			}
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		if (node.name.includes('-')) {
			return false; // we're setting all attributes on custom elements through properties
		}
	}


Review comment (suggestion):
should this share the logic from here?

https://github.com/sveltejs/svelte/blob/1e5a385c968afb302beb38e7d8dd2982fdd66d0c/packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js#L584

Code after:
		skipped -= 1;
		state.init.push(b.stmt(b.call('$.next', skipped !== 1 && b.literal(skipped))));
	}
}

/**
 * @param {SvelteNode} node
 * @param {ComponentContext["state"]} state
 */
function is_static_element(node, state) {
	if (node.type !== 'RegularElement') return false;
	if (node.fragment.metadata.dynamic) return false;
	if (node.name.includes('-')) return false; // we're setting all attributes on custom elements through properties

	for (const attribute of node.attributes) {
		if (attribute.type !== 'Attribute') {
			return false;
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (
			attribute.value !== true &&
			!is_text_attribute(attribute) &&
			// If the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template so before returning false we need to check that the attribute is not inlinable
			!is_inlinable_expression(attribute.value, state)
		) {
			return false;
		}
	}

	return true;
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js (JavaScript)

Code before:
		skipped -= 1;
		state.init.push(b.stmt(b.call('$.next', skipped !== 1 && b.literal(skipped))));
	}
}

/**
 * @param {SvelteNode} node
 * @param {ComponentContext["state"]} state
 */
function is_static_element(node, state) {
	if (node.type !== 'RegularElement') return false;
	if (node.fragment.metadata.dynamic) return false;

	for (const attribute of node.attributes) {
		if (attribute.type !== 'Attribute') {
			return false;
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.value !== true && !is_text_attribute(attribute)) {
			// if it's not a text attribute but it's an inlinable expression
			// we will inline it in the template so we can still consider it static
			return is_inlinable_expression(
				Array.isArray(attribute.value) ? attribute.value : [attribute.value],
				state
			);
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		if (node.name.includes('-')) {
			return false; // we're setting all attributes on custom elements through properties
		}
	}

	return true;
}


Review comment (bug):
Returning here with true could lead to bugs down the road in case a condition afterwards would return false

Code after:
		skipped -= 1;
		state.init.push(b.stmt(b.call('$.next', skipped !== 1 && b.literal(skipped))));
	}
}

/**
 * @param {SvelteNode} node
 * @param {ComponentContext["state"]} state
 */
function is_static_element(node, state) {
	if (node.type !== 'RegularElement') return false;
	if (node.fragment.metadata.dynamic) return false;
	if (node.name.includes('-')) return false; // we're setting all attributes on custom elements through properties

	for (const attribute of node.attributes) {
		if (attribute.type !== 'Attribute') {
			return false;
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (
			attribute.value !== true &&
			!is_text_attribute(attribute) &&
			// If the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template so before returning false we need to check that the attribute is not inlinable
			!is_inlinable_expression(attribute.value, state)
		) {
			return false;
		}
	}

	return true;
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js (JavaScript)

Code before:
		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (
			attribute.value !== true &&
			!is_text_attribute(attribute) &&
			// if the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template so if it's not inlinable we can return false but if it is inlinable (or a text)
			// attribute we keep count of the inlinable attributes so that if all the attributes are inlinable
			// we deem the node as static
			!is_inlinable_expression(
				Array.isArray(attribute.value) ? attribute.value : [attribute.value],
				state
			)
		) {
			return false;
		}
	}

	return true;
}


Review comment (suggestion):
I wonder if the method should take `Node | Node[]` so that we can just pass `attribute.value` rather than having to do `Array.isArray(attribute.value) ? attribute.value : [attribute.value]` both places we call it

Code after:
		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (
			attribute.value !== true &&
			!is_text_attribute(attribute) &&
			// If the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template so before returning false we need to check that the attribute is not inlinable
			!is_inlinable_expression(attribute.value, state)
		) {
			return false;
		}
	}

	return true;
}
Repository: sveltejs/svelte
File: packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js (JavaScript)

Code before:
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (
			attribute.value !== true &&
			!is_text_attribute(attribute) &&
			// if the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template so if it's not inlinable we can return false but if it is inlinable (or a text)
			// attribute we keep count of the inlinable attributes so that if all the attributes are inlinable
			// we deem the node as static
			!is_inlinable_expression(attribute.value, state)
		) {
			return false;
		}
	}

	return true;
}


Review comment (suggestion):
this would be easier to read if it's not all one long run-on sentence :laughing:

> If it is an inlinable (or text) attribute we keep count of the inlinable attributes and if all the attributes are inlinable then we deem the node as static

where does this happen? are you referring to what occurs within `is_inlinable_expression`? if so, I think we could remove " we keep count of the inlinable attributes" as it's not really counting them, but just checking them all. or were you referring to something else?

```suggestion
			// If the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template. If it's not inlinable we can return false. If it is an inlinable (or text)
			// attribute we keep count of the inlinable attributes and if all the attributes are inlinable
			// then we deem the node as static
```

Code after:
		}

		if (is_event_attribute(attribute)) {
			return false;
		}

		if (attribute.name === 'autofocus' || attribute.name === 'muted') {
			return false;
		}

		if (node.name === 'option' && attribute.name === 'value') {
			return false;
		}

		// We need to apply src and loading after appending the img to the DOM for lazy loading to work
		if (node.name === 'img' && attribute.name === 'loading') {
			return false;
		}

		if (
			attribute.value !== true &&
			!is_text_attribute(attribute) &&
			// If the attribute is not a text attribute but is inlinable we will directly inline it in the
			// the template so before returning false we need to check that the attribute is not inlinable
			!is_inlinable_expression(attribute.value, state)
		) {
			return false;
		}
	}

	return true;
}