Upgrading from 0.7.6 to 0.7.7 breaks Nested/Cumulative Layouts.

[Blankeos]

After upgrading from 0.7.6 to 0.7.7, layouts seem to be breaking.

I tried...
✅ [email protected] and [email protected]. It works. (using [email protected])
❌ [email protected] and [email protected]. It doesn't work. (using [email protected])
❌ [email protected] and [email protected]. It doesn't work. (using [email protected])

I also thought maybe it was just Vite, but no. Because:
❌ [email protected] and [email protected]. It doesn't work. (using [email protected])
✅ [email protected] and [email protected]. It works. (using [email protected])

So I kinda conclude It's because of [email protected].

Still investigating what the cause is... So opening this issue for that.
I'm trying to compare the changes from 0.7.6 to 0.7.7: 94bda74...16c9c82 (I just used the release: 0.7.6 and release: 0.7.7 commits).

Reproduction

I will add here when I can, but you can maybe use my boilerplate: https://github.com/blankeos/solid-hop (It's pretty barebones). And then reproduce the package versions I mentioned above.

[Blankeos]

So I noticed, it only breaks for Nested Layouts, that's when it shows the Maximum call stack size exceeded error.

Very consistent with the a project bootstrapped with Bati as well.

[brillout]

@rtritto @Blankeos Up for digging into this? It would be nice if the community can (eventually) take over vike-solid so that the Vike team can focus on other parts of Vike. There is also: https://github.com/vikejs/vike-solid/issues.

[Blankeos]

Yep looking into it. I'm getting somewhere:

Debugging Code

...

1. onMount

2. on page navigations... (I found the infinite recursive call)

Notice how i is supposed to be a number, but it seems to be a signal?, essentially it's recursively calling renderWrappers() multiple times

---

The fix is this (surprisingly simple)

I actually have no idea why "0" becomes a signal here. iirc, I also added a 0 as a default here as well in our initial PR for cumulative layouts, hence the call was just renderWrappers()

• (Link to old PR)
• (Link to File)

This might just be SolidJS shenanigans, when writing renderWrappers(), it becomes renderWrappers(() => 0) maybe? Again, idk. But an explicit renderWrappers(0) call fixes it. 😅

[Blankeos]

Hmmm weird. Don't mind the fix for now. I did fix the recursion, but...

I noticed there's another weird behavior:

Current Bug after the first fix:

layout-state-bug.mp4

Desired Behavior (but only in 0.7.6)

0.7.6.behavior.mp4

More findings...

I tried with 3 layouts. It seems it resets rendering for everything when the number of layouts change. Which tells me reconcile isn't working correctly. Will keep digging...

[Blankeos]

I discovered something really stupid with Javascript (or maybe Solid)

I noticed simply changing array indexing from .at to [], and the state actually gets persisted??

Not sure what's so fundamentally different between them because I genuinely thought they were the same!! Just that .at() has the ability to index backwards (like -1 as the last element).

So the fix on the second bug from #144 (comment). Is actually:

// Assuming you `.toReversed()` in the reconcile.
let item = wrappers.at(props.index) // ❌ bad
let item = wrappers[props.index] // ✅ good? 

// It also doesn't work if it wasn't reversed in the reconcile. (The state does not persist, and the component rerenders)
let item = wrappers.at(-(i + 1)); // ❌ bad.
let item = wrappers[layouts.length - 1 - index] // ❌ bad.

I bumped into this on a whim while fixing this bug.

I tried accessing backwards using [], but the problem actually still persists. Some solutions I tried:

    // Attempt 1:
    let item = wrappers[wrappers.length - 1 - i];

    // Attempt 2:
    const index = wrappers.length - 1 - i;
    let item = undefined;
    if (index !== -1) item = wrappers[index];

Takeaways:

• Use [] over .at() so Solid knows how to diff the existing component (and prevent rerender).
• Must .reverse() instead of accessing backwards.

Now works as expected:

expected.behavior.now.mp4

[magne4000]

Interesting findings. It would probably be a good idea to check if such issues are already known by the Solid team, and open some issues if that is not the case.

[rtritto]

Opened 2 related issues:

• Use [<index>] over .at(<index>) solidjs-community/eslint-plugin-solid#174
• Use reverse function over negative index solidjs-community/eslint-plugin-solid#175