Fix #708

[Rich-Harris]

This definitely isn't the answer, but I think it's heading in the right direction, so I figured I'd leave it here for tomorrow.

[TehShrike]

When I test in my repro repo with 2d2c68c, I get this error on build:

🚨   (svelte plugin) (5:24) Expected "
3: <ol>
4:   {{#each arrayToDisplay as id}}
5:     <Top bind:top="idToTop[id]">
                             ^
6:       <li>{{id}}: top is {{idToTop[id]}}</li>
7:     </Top>

[TehShrike]

Ignore that last message, I tried installing straight from Github and forgot about the build step.

When I check out this branch and build myself, and then use npm link to test my repro repo, everything works on the initial load - but when I add more items to the array by upping the number, those new items all have undefined values.

[Rich-Harris]

@TehShrike yep, was just looking into your previous comment, seeing the same thing. Thought I had a test that covered that. Investigating...

[Rich-Harris]

Gah, I'm close. Every time I fix one test, another fails. I thought I'd figured it out for a bit back there, but not quite.

Incidentally, that repro won't work even when this bug is fixed — oncreate only gets called for new components, and new components are appended to the list. So while the yield block for (say) the first component will go from id-2 to id-3 and so on, idToTop["id-3"] will never be defined (because every time you add a new component, its ID is id-0). Comment out the .reverse(), and you'll see the actual bug.

Once the bug is fixed, to get the behaviour you want you would need to use a keyed each block.

[TehShrike]

I had a suspicion that was related, which was why I through the reverse in there.

[Rich-Harris]

I think this is ready to go, if anyone is interested in taking a look. The basic gist is this: callbacks that need to happen in a particular order (setting up observers for bindings, oncreate hooks, and transitions) are all handled at the root component level, rather than happening in an unpredictable order at various different levels of the app tree.

This way, we can enforce the order (ensuring that a _beforecreate function doesn't trigger a secondary set(...) call and empty _beforecreate, _oncreate and _aftercreate prematurely) with a _block flag on the root component.

Should also fix #714.

[TehShrike]

Testing in my repro repo, everything is cool as long as I don't reverse the array or rearrange elements, which I understand is a different issue, so 👍

[stalkerg]

Where are you init options._root object?
Also, my approach looks like much simpler.

[Rich-Harris]

options._root is part of the initialisation options when Svelte is rendering a nested component.

Code always looks simpler when you wrote it 😀 This PR is more comprehensive, which is why it looks more complex — it makes the order of operations more explicit and predictable, and avoids passing down additional information on each update. The test in #715 passes in this branch, whereas the test in this PR doesn't pass in the other branch

[stalkerg]

options._root is part of the initialisation options when Svelte is rendering a nested component.

now, I understand. It's really similar approach, I mean _block = true. But maybe better use _domBlock or _blockDom name? Because we have already "block" in another sense.

The test in #715 passes in this branch, whereas the test in this PR doesn't pass in the other branch

yep, I think you should merge this PR and reject mine.

[Rich-Harris]

Maybe _lock instead of _block. I'll change that, thanks. Probably not _domBlock because it's not purely about DOM updates, it's about e.g. preventing oncreate handlers firing prematurely (which could be doing non-DOM-related things like firing AJAX requests, with incorrect parameters if they happen out of order).