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

Emit error events rather than throwing #387

Merged
merged 9 commits into from
Jan 11, 2025
Merged

Conversation

jtbandes
Copy link
Member

Changelog

Added an "error" event to the LabelPool and FontManager.

Docs

None

Description

Rather than throwing errors, emit them via an "error" event.

For FG-9839

@jtbandes jtbandes marked this pull request as ready for review December 20, 2024 01:32
@jtbandes jtbandes requested review from achim-k and snosenzo December 20, 2024 01:32
Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

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

Would it be possible/worth it to have a simple test for this?

src/FontManager.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

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

What's the preference for dispatch error vs throw error? I feel like throw error gives more traceabability as to which call threw the error albeit certainly at the cost of needing to wrap functions that may error in a try catch. Dispatching makes the errors themselves a bit more of an implicit requirement to handle rather than more explicit or in your face. I think I prefer throwing errors, but I'm not totally against this

@jtbandes
Copy link
Member Author

jtbandes commented Jan 6, 2025

at the cost of needing to wrap functions that may error in a try catch

This was my issue with it -- especially when such functions are "non-obvious" that they may throw errors, like label.setText("foo"). It felt like that was such a basic function that it kinda made sense to not throw. The related change which I guess I didn't call out in the description is that this makes the error non-fatal, i.e. you will see some � boxes but it will render as many characters as it can rather than totally bailing out.

I could also see maybe some implementation that allows swapping between multiple atlas textures once we hit the size limit. Didn't feel like it was worth the complexity though. In practice it's very rare for labels to have so many unique characters and only happened in this case because a user was passing in corrupt string data.

@snosenzo
Copy link
Contributor

snosenzo commented Jan 7, 2025

you will see some � boxes but it will render as many characters as it can rather than totally bailing out.

I guess as long as it's evident that an error is occurring in some way then it's fine. Another option to make it less implicit would be to require passing an onError method to the constructor to handle these errors?
Either way I think it's fine though

@jtbandes jtbandes merged commit bc8641f into main Jan 11, 2025
4 checks passed
@jtbandes jtbandes deleted the jacob/dispatch-errors branch January 11, 2025 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants