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

Added error and stack trace parameters to onError #35

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonjomckay
Copy link
Contributor

This adds the thrown error and stackTrace parameters to the onError callback.

It's quite useful to have these passed in, so the caller can e.g. show a SnackBar with some of the error details.

@stargazing-dino
Copy link
Owner

Man, I was not paying attention when I added that callback. Good catch !

I think there might already be a flutter typedef that is (Object? error, Stacktrace? stack) but I might be wrong. Anyways, will look at this when I get home. Also, you might want to pull to resolve conflicts as I merged your other pr

@stargazing-dino
Copy link
Owner

stargazing-dino commented Jun 15, 2022

Currently our callbacks (onSuccess, onError) fire at the end of the errorDuration/successDuration so they might mislead the user to think they'll fire as soon as an error is encountered. I'm not sure when I added these callbacks but I think they should be renamed to onSuccessCompleted or onErrorShown etc to show they're at the end of error/loading state. For immediately telling if it's an error or success, you could do it in the async onPressed no?

What are your thoughts on this

@jonjomckay jonjomckay force-pushed the add-error-and-stacktrace-to-onerror branch from 6033d65 to def3dcf Compare June 16, 2022 10:33
@jonjomckay jonjomckay changed the title Fixed onError never being called when an error occurs Added error and stack trace parameters to onError Jun 16, 2022
@jonjomckay
Copy link
Contributor Author

I couldn't find a typedef in Flutter itself that defines (Object? error, Stacktrace? stack), but I'm happy to change it if you can point me towards it :)

Do onError and onSuccess need to fire after the defined durations, or can they fire straight away? I see the code is used to wait for a bit before running the callback, then resetting the icon back to the idle one, but they could be split into two?

If it helps, I'm using onError and onSuccess to display snackbars with appropriate messages after the button action is performed. For my use case, I'd prefer the callbacks to run as soon as an error or success happens, but I'd still want the button icon to change gracefully, like it currently does. The reason I'd like to use these callbacks is so I don't have to litter my code with duplicate-y try/catch statements, and can instead just pass a method reference.

I'll leave it in your hands to think over, but let me know if you want me to make any changes!

@jonjomckay jonjomckay force-pushed the add-error-and-stacktrace-to-onerror branch from def3dcf to 623dd7e Compare June 16, 2022 10:49
@stargazing-dino
Copy link
Owner

stargazing-dino commented Jun 16, 2022

Yeah no worries with the typedef. What you have currently looks good.

So I think what is best is to rename the current callbacks we have now to onSuccessShown and onErrorShown. That would show the user those callbacks happen after the specified duration. I imagine these callbacks being used to route users or similar so I think they have utility.

After that, we should add back in onError and onSuccess but so that they're immediate.

                    timer?.cancel();

                    onPressed.call().then((_) {
                      completer.complete();
                      onSuccess?.call();

                      if (mounted) {

I think the docs should better reflect how these callbacks are called as well since what I have currently doesn't make sense.

If you want to do both the changing and additions in this PR or only the renaming, however is fine. Thank you for the help too !

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

2 participants