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

outtroBlock issue causes dynamic components with each block to not be destroyed #1660

Closed
btakita opened this issue Aug 16, 2018 · 8 comments
Closed

Comments

@btakita
Copy link
Contributor

btakita commented Aug 16, 2018

My app seems to be running into this edge case. I don't yet have an isolated reproduction of the issue, but I do have some generated output that has the bug with the fix.

This causes the issue. The problem is each_blocks[i] is set to null & outroBlock is called two times. The first invocation does not have the fn argument and the second invocation does (however, each_blocks[i] is null).

function outroBlock(i, detach, fn) {
	if (each_blocks[i]) {
		each_blocks[i].o(function () {
			if (detach) {
				each_blocks[i].d(detach)
				each_blocks[i] = null
			}
			if (fn) { fn() }
		})
	}
}

This seems to work:

function outroBlock(i, detach, fn) {
	if (each_blocks[i]) {
		each_blocks[i].o(function () {
			if (detach) {
				each_blocks[i].d(detach)
				each_blocks[i] = null
			}
			if (fn) { fn() }
		})
	} else {
		if (fn) { fn() }
	}
}
@btakita
Copy link
Contributor Author

btakita commented Aug 16, 2018

#1512

@btakita
Copy link
Contributor Author

btakita commented Aug 17, 2018

I attempted to reproduce the issue. However, there is another error that occurs when you click 'Child2'

TypeError: switch_instance is undefined

TypeError: if_blocks[current_block_type_index] is undefined

https://svelte.technology/repl?version=2.11.0&gist=1942fddd9ef996aa566059a90b07df29

From my app, it seems like the issue has to do with the following:

if/else block or dynamic component
loads a child component
loads a grandchild component
has an each block which loads a great grandchild component

If the each block in the grandchild component does not load a component, then the issue does not manifest.

@btakita
Copy link
Contributor Author

btakita commented Aug 17, 2018

The order of method invocation is as follows:

for (; #i < ${iterations}.length; #i += 1) ${outroBlock}(#i, 1);

for (; #i < ${iterations}.length; #i += 1) ${outroBlock}(#i, 1);

then

for (let #i = 0; #i < ${iterations}.length; #i += 1) ${outroBlock}(#i, 0, ${countdown});`

for (let #i = 0; #i < ${iterations}.length; #i += 1) ${outroBlock}(#i, 0, ${countdown});`

Notice that the first time outroBlock is invoked, there is no fn passed in to outroBlock. The first invocation causes each_blocks[i] to be set to null, which prevents the countdown fn from being called. The PR calls the fn if passed in even if each_blocks[i] is null.
I'm still attempting to recreate this case in the REPL, but it definitely occurs in my app...

@btakita
Copy link
Contributor Author

btakita commented Aug 17, 2018

In the short term, setting

nestedTransitions: false

causes outroBlock to not be rendered.

btakita added a commit to ctx-core/dev that referenced this issue Aug 17, 2018
	fix(@ctx-core/svelte): Fix component not being destroyed due to svelte bug
		sveltejs/svelte#1660
@Rich-Harris
Copy link
Member

I fixed the issue that was preventing the reproduction, in #1677. Any chance you can create a repro now that that's released? Your fix in #1662 looks good, but it'd be great to have a repro if possible so we can guard against regressions.

@jacwright
Copy link
Contributor

#1693 and #1709 contain fixes for each blocks (the latter threw an error in the console, so I'm less sure that was for this issue). Agreed a repro would be nice to know if we can close this.

ekhaled pushed a commit to ekhaled/svelte that referenced this issue Sep 26, 2018
Allow non-existent dynamic components to be destroyed
@Rich-Harris
Copy link
Member

There was never a repro for this — is it still an issue, or was it fixed?

@Conduitry
Copy link
Member

I'm going to assume this has been fixed, at least by the recent outro/detach stuff. Please reopen if you have a repro.

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

No branches or pull requests

4 participants