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

#2668 - Add null-checks in on_outro callback for IfBlock #2717

Closed
wants to merge 6 commits into from

Conversation

IOuser
Copy link

@IOuser IOuser commented May 8, 2019

@Rich-Harris
Copy link
Member

Thanks, this is an excellent test case and a compact fix. It feels like there might be a deeper bug here — normally, I don't think it's possible to have 'nested' groups of outros. At least, if I modify transitions.js like so...

+let grouping = false;
+
export function group_outros() {
+	if (grouping) {
+		throw new Error('already grouping');
+	}
+
+	grouping = true;
+
	outros = {
		remaining: 0,
		callbacks: []
	};
}

export function check_outros() {
+	grouping = false;
+
	if (!outros.remaining) {
		run_all(outros.callbacks);
	}
}

...then the only test that fails is the one in this PR. So I wonder if there's an alternative fix that renders the null check unnecessary? (Unfortunately my brain is only capable of asking the question right now, not delivering the answer. It's very possible that I'm barking up the wrong tree.)

@IOuser
Copy link
Author

IOuser commented May 10, 2019

@Rich-Harris, thank you for reply.
This snippet brought me to another fix without null checks. I tried to add counter for group_outros and check_outros. This allows to avoid override outros for 'nested' groups of outros. And also this allows to run callbacks only when 'outer' check_outros being called.

+let n = 0;
+
export function group_outros() {
+	if (n++ > 1) {
+		return; // already grouping
+	}
+
	outros = {
		remaining: 0,
		callbacks: []
	};
}

export function check_outros() {
+	if (n-- > 1) {
+		return; // skip inner calls
+	}
+
	if (!outros.remaining) {
		run_all(outros.callbacks);
	}
}

In that case tests passed without null checks in outro callbacks.

But unfortunately in that case 'out' animations are broken because nested blocks are still not detached when create_animation calls and it returns noop =)

I will continue investigating and share example with animations in separate repository a little bit later.

@IOuser
Copy link
Author

IOuser commented May 14, 2019

I made a repository with an list animations example.
I think best solution for this issue - reset outros object after run_all callbacks.
I'm also added test case for multiple branches IfBlock.

@nikku
Copy link
Contributor

nikku commented Jun 3, 2019

@IOuser is this ready to be looked into? @Rich-Harris Could you give this PR another look?

@Rich-Harris
Copy link
Member

The underlying issue is closed, having (apparently) been fixed by #3172, so I'll close this PR. 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
Development

Successfully merging this pull request may close these issues.

3 participants