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: Dispatched errors from inside components bubble to top #4786

Closed
wants to merge 4 commits into from

Conversation

aliabid94
Copy link
Collaborator

dispatch("error") stopped working during an error modal refactor, since we now handle errors at the top. Fixed.

@aliabid94 aliabid94 requested a review from pngwn July 4, 2023 16:22
@@ -417,6 +418,19 @@
handled_dependencies[i].push(id);
});
});
if (!attached_error_listeners) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently handle_mount runs multiple times, so I had to use attached_error_listeners to make sure I only attach error listeners once. Could not figure out why handle mount runs multiple times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's by design, because onMount from child components will run at different times with different elements being ready at different points. So we need to allow this onMount callback to rerun as many times as is necessary to ensure that we handle the mounting of all child components, that said I wanna reword this when I refactor this code shortly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So onMount runs every time a child is added?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. That's why we keep track of which dependencies have already been handled in this block, so we don't add additional listeners for them after we have done it once. We track them via their ids.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, did something similar

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attached_error_listeners is an array of ids now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will re-review.

@aliabid94 aliabid94 changed the title Fix: Dispatched errors bubble to top Fix: Dispatched errors from inside components bubble to top Jul 4, 2023
@gradio-pr-bot
Copy link
Contributor

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4786-all-demos

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Jul 4, 2023

Chromatic build successful 🎉

Comment on lines 422 to 433
attached_error_listeners = true;
components.forEach((c) => {
if (c.instance) {
c.instance.$on("error", (event_data: any) => {
messages = [
new_message(event_data.detail, -1, "error"),
...messages
];
});
}
});
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this potentially only attach a single listener when there could be multiple?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So instead of only running this once, every time we should track which components have an error listener attached, and add to the ones that don't, correct?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry (missed this earlier).

@vercel
Copy link

vercel bot commented Jul 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
gradio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 5, 2023 6:32pm

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for fixing this @aliabid94!

@aliabid94 aliabid94 closed this Jul 6, 2023
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.

None yet

3 participants