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

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

Closed
warmrobot opened this issue Jan 14, 2020 · 12 comments · Fixed by #4507 or #4508
Closed

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

warmrobot opened this issue Jan 14, 2020 · 12 comments · Fixed by #4507 or #4508
Assignees
Labels

Comments

@warmrobot
Copy link
Contributor

warmrobot commented Jan 14, 2020

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:

  • Your browser and the version: (e.x. Chrome 52.1, Firefox 48.0, IE 10)
    FF, Chrome

  • Your operating system: (e.x. OS X 10, Ubuntu Linux 19.10, Windows XP, etc)
    Mac

  • Svelte version (Please check you can reproduce the issue with the latest release!)
    3.17.0

  • Whether your project uses Webpack or Rollup
    both reproduce bug

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
Copy link

ghost commented Jan 14, 2020

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
Copy link
Contributor Author

warmrobot commented Jan 14, 2020

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
Copy link

ghost commented Jan 14, 2020

@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.

@Conduitry Conduitry added the bug label Jan 19, 2020
@warmrobot
Copy link
Contributor Author

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

@alexkornitzer
Copy link

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
Copy link
Member

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

@alexkornitzer
Copy link

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
Copy link

@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
Copy link
Member

Thanks. Let me look into it

@alexkornitzer
Copy link

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

@tanhauhau
Copy link
Member

Cheers

@warmrobot
Copy link
Contributor Author

Thank you so much!

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