sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
79.71k stars 4.23k forks source link

Wrong {#if} {:else} with context_overflow (this.context.length > 31) #4263

Closed warmrobot closed 4 years ago

warmrobot commented 4 years ago

Describe the bug if directive doesn't work as expected if context_overflow (this.context.length > 31)

To Reproduce https://svelte.dev/repl/09bca24d4cc940e38b00afe53ad3da51?version=3.17.0

Expected behavior You should see true two times

Information about your Svelte project:

Severity blocking an upgrade to Svelte with bitmasks

Additional context Problem in this check dirty[0] it is falsy

function select_block_type(ctx, dirty) {
            if (dirty[0] & /*a0*/ 1) show_if = !!(/*arr*/ ctx[1].indexOf(/*a1*/ ctx[2]) && /*a0*/ ctx[0] <= 1);
        }

May be dirty should be mutidimensional array? Because it is not

ghost commented 4 years ago

Thanks for the report!

This is a strange one indeed because it requires all four of these conditions:

  1. 30+ variables declared
  2. A function that alters one of the variables, a0 in this instance
  3. referencing the const arr and altered variable (a0) in a conditional
  4. Having an out: transition somewhere in the {#if} or {:else} block

It boils down to missing show_if === null in the conditional for selecting the if/else block.

As for your observation of dirty array, no it is a bitmask array representing 30 items per block (-1 is special reserved value). In this case, you have exactly 30 variables so it fits in a single value.

warmrobot commented 4 years ago

I spend a day to debug it. And I will try to simplify the example (I am sure it is possible). Right now I supposed that compiler must send [-1], instead of -1 in "context_overflow mode" ) изображение

context_overflow in the right

ghost commented 4 years ago

@warmrobot,

Very curious, I don't see the use of dirty[0] when it fails. https://svelte.dev/repl/a895c06b0bdf4bceaeb1af406263b2b1?version=3.17.0

Your failing example will work good for debugging the compiler, I just removed some of the extra text so that it is easier to put into a failing test.

warmrobot commented 4 years ago

I add a failing test of hydration in forked https://github.com/warmrobot/svelte Still investigating in https://github.com/warmrobot/masks

alexkornitzer commented 4 years ago

I have just hit this error as well, I have one case which is always going to be false: image And I have the case as already mentioned in above comments where it thinks dirty is an array even though it isn't: image

Has there been any progress on this regression, or will we just have to pin to 3.15.0 for the foreseeable future?

tanhauhau commented 4 years ago

@AlexKornitzer it'll be helpful if you could provide us a repro in the https://svelte.dev/repl

alexkornitzer commented 4 years ago

Let me see if I can get it in the REPL, was struggling to do that earlier due the amount of complexity required to reproduce it and it seemed like the OP had identified the issue. In my code the ctx index is going above 30. Anyway let me see if I can abstract my table component into the REPL.

alexkornitzer commented 4 years ago

@tanhauhau so I used the original example to bump the context over 30, removed a lot of the table fluff as well cause I guess at the end of the day its the same issue as above. I doubt I'll be able to hit the 0 dirty issue without knowing exactly how the bitmasker works. Anyway so we have the broken example (you will see that pagination works but the tbody will not update): https://svelte.dev/repl/908720c4e7404cb193075b294604e333?version=3.19.1 If we then pin to 3.15.0 all is working: https://svelte.dev/repl/908720c4e7404cb193075b294604e333?version=3.15.0

tanhauhau commented 4 years ago

Thanks. Let me look into it

alexkornitzer commented 4 years ago

Rapid, thanks guys, can confirm all is working now :D

tanhauhau commented 4 years ago

Cheers

warmrobot commented 4 years ago

Thank you so much!