Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix else-block update in keyed each-block #4552

Closed

Conversation

rproserpio
Copy link

@rproserpio rproserpio commented Mar 13, 2020

This fixes #4536 and fixes #4549.
The value of the updated array was in scope only in the each block and not in the else block.
The bug was likely introduced in #4413.
I've chosen to remove the shadowing of the this.vars.each_block_value variable to keep the behavior of the unkeyed case i.e. the let binding in the fragment initialization is updated during the update.

Thanks to @bwbroersma for the help and test case.

@rproserpio rproserpio changed the title Fix else-block update in keyed each-block #4536 and #4549 Fix else-block update in keyed each-block Mar 13, 2020
@Conduitry
Copy link
Member

I'm not positive this is the correct solution - or, at least, it doesn't seem to precisely undo the problem that was introduced in #4413. Comparing the output in 3.18 and 3.19, I see that even in 3.18, we were declaring a new variable for the each value here, not using the block/fragment-scoped one.

I'm not sure what other effects overwriting the higher-up variable would have, but what seems to me to be more in the spirit of what was happening before is to just move the const ${this.vars.each_block_value} = ${snippet}; outside the scope of the new if block.

@tanhauhau do you have any thoughts on this?

@tanhauhau
Copy link
Member

@Conduitry found the root cause, created a PR #4558

@rproserpio
Copy link
Author

Oh, nice. I'm closing this PR, then. Thank you

@rproserpio rproserpio closed this Mar 14, 2020
@rproserpio
Copy link
Author

By the way, the let binding in the fragment scope keeps a reference of the array as initialized. I don't think it's needed or useful as it could be modified by a pattern like

list.push(el)
list = list

@rproserpio rproserpio deleted the failing-each-block-keyed-else branch March 14, 2020 13:48
@tanhauhau
Copy link
Member

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants