fix: avoid hoisting error by using 'let' instead of 'var'

[craig-jennings]

Fixes #11284

There was a small bug introduced by #11230 where it referenced value within a closure, but value was declared with var instead of let. This created a situation where value was hoisted and thus any changes to value would be propagated to the closure rather than it maintaining the intended value. Using let instead of var here keeps the scoping of value correct for the closure.

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

[changeset-bot]

🦋 Changeset detected

Latest commit: 583de86

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dummdidumm]

Thank you!

[dummdidumm]

just realized the same bug happens for opts and event_name, they need to be let, too

[harrisi]

I think this can still cause an issue in the future because key is hoisted for the loops.

A more minimal fix that is more robust is not hoisting key and doing the value change.

Although, this is the second bug related to var recently. Maybe a good sign to stop using var.

[craig-jennings]

@harrisi While key is indeed hoisted, it doesn't make much of a difference here because key isn't passed into another lexical scope. event_name/value/opts were unique in that they were passed into a closure so their scoping was important, otherwise the closure would access the wrong values when executed.

Though, I do agree that var has its problems and let/const are probably preferable. I'm not sure about any performance implications between their usages though, and I know this project is (rightfully) quite focused on that.

[MrHBS]

Yeah I have always wondered why this project extensively uses var.

[Rich-Harris]

var is faster, because there's no need to check whether you're in a temporal dead zone

[7nik]

Unfortunately, efficient code sometimes requires weird stuff.
For comparison, TS was getting up to 15% burst after switching to vars.

[harrisi]

In that issue you can see that the performance difference is considered a bug in v8. There's an open issue about it.

Regardless, this feels like the wrong place for this discussion. I may open an issue at some point about it, since it feels like it goes against most of the tenets Rich posted about.

[dummdidumm]

It does not. This is the nitty gritty nasty details that allow you to write nice performant Svelte code and has nothing to do with the public-facing API