Starting with v3.39.0 sorting a keyed each block messes up the DOM

[pablote]

Describe the bug

I have a keyed each block displaying items. These items can sorted by two different criterias, whenever I resort the items, the DOM of the individual items is changed in a strange way. Div elements within the items are displayed out of order.

This works fine up to version 3.38.3, and starts failing with 3.39.0. It also fails with versions 3.40.0 and 3.40.1.

Reproduction

I'm attaching some screenshots. Here you can see the code as it's seen on the IDE, and then how it's shown on the browser dev tools. Notice how in the browser metrics__title is shown last, while in the IDE it's the first element within an item.

I haven't been been able to reproduce this on the REPL, but here's one that is as close as I could with a simple example: https://svelte.dev/repl/6ef13760c89f4fd7af060d0312ca1a71?version=3.40.1

I know this is not ideal, but maybe since I can pin point this to a specific version, it can help narrow down the problem. Removing the key to the each block also fixes the problem, but I need that for animate:flip to work.

Logs

No response

System Info

➜  web git:(develop) ✗ npx envinfo --system --npmPackages svelte,rollup,webpack --binaries --browsers
Need to install the following packages:
  envinfo
Ok to proceed? (y)

  System:
    OS: macOS 11.4
    CPU: (8) x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz
    Memory: 39.97 MB / 16.00 GB
    Shell: 5.8 - /usr/local/bin/zsh
  Binaries:
    Node: 16.2.0 - /usr/local/bin/node
    Yarn: 1.13.0 - /usr/local/bin/yarn
    npm: 7.19.1 - /usr/local/bin/npm
    Watchman: 2021.06.07.00 - /usr/local/bin/watchman
  Browsers:
    Chrome: 92.0.4515.107
    Firefox Developer Edition: 91.0
    Safari: 14.1.1
  npmPackages:
    svelte: ^3.38.3 => 3.40.1

Severity

blocking an upgrade

[pablote]

Some more info. I can confirm that the elements wrongly placed after sorting are conditionally rendered. If I comment out the if around the title, the problem is fixed, for the title at least. There's still problems with other elements though.

I'm still trying to repro this on a simplified REPL btw, with no success.

[pablote]

@Conduitry I've been able to reproduce this with a simple example, thing is, apparently this only happens when using Vite with the @sveltejs/vite-plugin-svelte plugin. Would this still be the right place to report this or should it be reported somewhere else?

This is the repro code (though it works on the REPL): https://svelte.dev/repl/e25e9d00d43e4d6eaadf5f3362c47f92?version=3.40.1

This is how it looks when using Vite:

[bluwy]

If you're able to repro with the latest version of vite-plugin-svelte, I'd say we should move this to https://github.com/sveltejs/vite-plugin-svelte/issues

[dominikg]

Reproduction with plain svelte template: https://github.com/dominikg/svelte-hydration-reorder-bug

The issue is caused by hydratable: true compiler option which vite-plugin-svelte sets by default.

[dominikg]

Also note that if you remove the wrapping {#if item.name} in Item.svelte , it won't reorder

[tanhauhau]

@hbirler @daybrush can help take a look on this issue?

[hbirler]

I believe https://github.com/hbirler/svelte/tree/append-fix might be the fix.
The commit: hbirler@32ce595

[dominikg]

updated repro to 3.41, still happening. https://github.com/dominikg/svelte-hydration-reorder-bug

@hbirler are you sure append is the right fix? this one not happening on initial hydration but when keyed children of an each loop are reordered. i'd not expect hydration to run again at that point

[hbirler]

@dominikg I believe the hydration version of the append function is still called after the initial hydration. They are only called with is_hydrating set to false. I believe that the issue is in the path that the hydration append function takes when is_hydrating is set to false.

[hbirler]

It seemed to fix the issue for me locally. Does it not work for you? Maybe I made a mistake.

[hbirler]

Additionally, the new non-hydration append and insert functions seem to not have checks to see if the node is already in the correct position. Calling appendChild or insertBefore with a node resulted in CSS animations always getting reset on my browser (https://jsfiddle.net/j0unmcyb/), even if the node is already in the correct position. The original hydration pull request (#6395) was written to address such animation resets/flickers (#4308) so hydration needs to keep these checks. So it might make sense to add these checks to the new non-hydration functions as well so that there is no difference between:

• post hydration, with hydration enabled
• hydration disabled

[tanhauhau]

@hbirler the change make sense, would you mind creating a pull request with a new test case 🙏🏻 ?

[Conduitry]

With the release of #6602 in 3.42.1, this should hopefully be fixed now. Thanks @hbirler!