Alpine: Regression in x-for

[bep]

This error came after 9e91c4704638ee5c3472eb8a19d54fcd0b5caa59

I don't have a codepen for this, but the component that fails has 3 nested x-for loops.

alpine.js:418 Uncaught TypeError: Cannot convert undefined or null to object
    at alpine.js:418
    at Proxy.forEach (<anonymous>)
    at handleForDirective (alpine.js:404)
    at alpine.js:1495
    at Array.forEach (<anonymous>)
    at Component.resolveBoundAttributes (alpine.js:1450)
    at Component.updateElement (alpine.js:1426)
    at alpine.js:1394
    at alpine.js:1358
    at walk (alpine.js:85)

[ryangjchandler]

Is there any chance you could provide a CodePen or sandbox? No worries if not, it just makes debugging faster and easier :)

I also assume you've got your CDN pointed to master since those refactoring changes haven't been tagged for release yet.

[bep]

I also assume you've got your CDN pointed to master

I have my own module/build system, but yes -- I did use the correct version.

I will try to find the time to recreate this in a code pen, I just thought I open an issue if someone more familar with the code would understand why this line was undefined:

https://github.com/alpinejs/alpine/blob/master/dist/alpine.js#L418

[ryangjchandler]

Yeah, I'm looking into it now :)

[SimoTod]

It shouldn't be due to nested fors. Looking at lookAheadForMatchingKeyedElementAndMoveItIfFound it seems that it doesn't return anything when it doesn't find a matching key.
IT can probably happen with a normal x-for if we insert a new keyed element at position one.

[ryangjchandler]

Yeah, I'm wondering if the simplest fix is wrapping it in an if statement. That might affect the code below that sets it to the current element too. On mobile at moment so sorry if I get this wrong.

[SimoTod]

It could be the right solution but i haven't dug into the new code that much, yet. I think we should compare it to the old code and see what has changed and why. I'll try to have a look tomorrow after work if nobody has time to look into it before.

[ryangjchandler]

Yeah, agreed.

[ryangjchandler]

I'm not gonna get a chance to look at this or any other issue for a little while.

[SimoTod]

I believe it's just a matter of checking if nextElem exists at line 411 and, if it doesn't, calling
nextEl = addElementInLoopAfterCurrentEl(templateEl, currentEl);.
The old code was doing something like that but I'm not sure if it has been changed for a reason.
Probably @calebporzio is better placed to fix the regression.

Here a minimal codepen to reproduce the issue -> https://codepen.io/SimoTod/pen/KKddWJK

[HugoDF]

@SimoTod must have slipped through the cracks when doing the refactor

could we create a PR with a failing test?

[SimoTod]

@HugoDF I won't have much time today. Feel free to fix it.

[muzafferdede]

i went through and collect some information that might help you to figure out faster.
nextEl = lookAheadForMatchingKeyedElementAndMoveItIfFound(nextEl, currentKey)
at this line it's assigned to undefined because:

function lookAheadForMatchingKeyedElementAndMoveItIfFound(nextEl, currentKey) {
    // If the the key's DO match, no need to look ahead.
    if (nextEl.__x_for_key === currentKey) return nextEl

    // If the don't, we'll look ahead for a match.
    // If we find it, we'll move it to the current position in the loop.
    let tmpNextEl = nextEl

    while(tmpNextEl) {
        if (tmpNextEl.__x_for_key === currentKey) {
            return tmpNextEl.parentElement.insertBefore(tmpNextEl, nextEl)
        }

        tmpNextEl = (tmpNextEl.nextElementSibling && tmpNextEl.nextElementSibling.__x_for_key !== undefined) ? tmpNextEl.nextElementSibling : false
    }
}

lookAheadForMatchingKeyedElementAndMoveItIfFound function spouse to find an element in the end but while loop fails to return because tmpNextEl in the end becomes false and it returns nothing. So nextEl becomes undefined.

Just for testing, i return back the nextEl at very end of the function like so:

function lookAheadForMatchingKeyedElementAndMoveItIfFound(nextEl, currentKey) {
...
...
...
        tmpNextEl = (tmpNextEl.nextElementSibling && tmpNextEl.nextElementSibling.__x_for_key !== undefined) ? tmpNextEl.nextElementSibling : false
    }
    return nextEl
}

and Append and Prepend worked normal.

So i guess it might be a logic issue?

update
something went wrong here : https://youtu.be/qkjKSZV9csA?t=7681

[SimoTod]

Yeah, it's a logical issue indeed but I'm not sure about returning nextEl.
You would update an element whose key doesn't match the one you are looking for.
The end result would contain all items but, if you have elements which need to preserve a state like an input field, you would shuffle them around. See https://codepen.io/SimoTod/pen/dyYYqez (try to type something in the inout fields and then click prepend, the input values won't move as they should).

I think we are okay to return a falsy value and do the insert at line 411 as I mentioned above since, not having matched a key, the element is a new one. This is the same codepen with the proposed fix:
https://codepen.io/SimoTod/pen/NWGGLmV
We should write some proper tests for this case before fixing it, though.

[bep]

With such a deliberate function name as lookAheadForMatchingKeyedElementAndMoveItIfFound I would assume that the logical flaw is the handling of the (non) return value. That also matches the functional behaviour (it works, but it throws).

[SimoTod]

Feel free to review ^