svelte icon indicating copy to clipboard operation
svelte copied to clipboard

fix: infinite loop caused by having bind:this in an each loop

Open ngtr6788 opened this issue 1 year ago • 2 comments

Fixes #4317

I did some digging and the infinite loop occurs when the array or iterable in the each-loop is a literal array, a (pure or impure, it depends) array from a regular function or generator function (iterable), containing objects or more arrays. The exact internal reasons on how the infinite loop came to be is ~confusing and hard to describe, even for me digging this 😁, so, uh, yeah...~

Here's my attempt to explain what's going on with the bug. I'll use this https://svelte.dev/repl/c0a3494b41f14ebd84a5f2f3f9747fa1?version=3.59.1, courtesy of the original bug report, as an example.

When the component is created and mounted, the binding functions are called (from the example, it is the div_binding functions), which add callbacks to the binding_callbacks list. After the component is mounted, the internal flush() function is called. The callbacks in bindings_callbacks are called, which invalidates the working_array. When it invalidates the working_array, it causes the component to update. When updating, we compare each array element with the previous array element, decide that they're different, which causes us to call the div_binding functions again, adding callbacks to the binding_callbacks list, again, and we're back to where we started, causing an infinite loop. (Thanks browser debugger!)

To get the infinite loop, at the stage where we compare the array elements, they are different. This could be caused by having a function returning an array of objects/arrays, or an impure function returning different things every time you call it, or a literal array of objects, or whatever. We comparing two objects (different in memory but same in what it contains), or two different values because the function is impure and we erroneously call the function that returns the array but the function is not invalidated. We don't need to do this, because we are not invalidating the actual array to be iterated, and requiring us to get the new array. Therefore, this PR adds a dirty check around arrays, or if the array is literal, no need to get a new literal array because we already defined it already.

Hope this comment is clear enough!

HEADS UP: BIG RESTRUCTURING UNDERWAY

The Svelte repo is currently in the process of heavy restructuring for Svelte 4. After that, work on Svelte 5 will likely change a lot on the compiler aswell. For that reason, please don't open PRs that are large in scope, touch more than a couple of files etc. In other words, bug fixes are fine, but feature PRs will likely not be merged.

Before submitting the PR, please make sure you do the following

  • [x] 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
  • [x] Prefix your PR title with feat:, fix:, chore:, or docs:.
  • [x] This message body should clearly illustrate what problems it solves.
  • [x] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with pnpm test and lint the project with pnpm lint

ngtr6788 avatar May 31 '23 13:05 ngtr6788

@ngtr6788 is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar May 31 '23 13:05 vercel[bot]

Thanks for adding support for XYZ. That was exactly what I needed!

yycdd456 avatar Jun 25 '23 09:06 yycdd456

Deployment failed with the following error:

There is no GitHub account connected to this Vercel account.

vercel[bot] avatar Aug 05 '23 17:08 vercel[bot]

⚠️ No Changeset found

Latest commit: 4855c27ab29c45bcef616d0202bdcb6a3bca1adc

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

changeset-bot[bot] avatar Aug 06 '23 15:08 changeset-bot[bot]

Will close this PR because of Svelte 5

ngtr6788 avatar Dec 19 '23 01:12 ngtr6788