felangel / bloc

A predictable state management library that helps implement the BLoC design pattern
https://bloclibrary.dev
MIT License
11.87k stars 3.4k forks source link

refactor: stop losing the original stack trace from dart 2.16 #2995

Open PlugFox opened 3 years ago

PlugFox commented 3 years ago

You must stop losing the original source stack trace like this (just for example):

https://github.com/felangel/bloc/blob/a3a9b2dba6546480d4ea26931ed024f274b9a7a9/examples/flutter_firebase_login/packages/authentication_repository/lib/src/authentication_repository.dart#L206-L210

it's big trouble, to read exceptions without source stacktraces, especially in crashlytics or sentry like logs

from dart 2.16 you must use Error.throwWithStackTrace like this:

//@dart=2.16

...

} on FirebaseAuthException catch (e, stackTrace) {
  Error.throwWithStackTrace(
    SignUpWithEmailAndPasswordFailure.fromCode(e.code),
    stackTrace,
  );
}

also, instead:

} catch (_) {

you must write:

} on Object {

or:

} on Object catch (error, stackTrace) {
felangel commented 3 years ago

Hi @PlugFox 👋 Thanks for opening an issue!

I agree we should improve the examples to preserve the stackTraces but I don't think we should use Dart 2.16 APIs in the examples because they are meant to run on the Flutter stable channel.

also, instead:

} catch (_) {

you must write:

} on Object {

or:

} on Object catch (error, stackTrace) {

I'd prefer to use:

on CustomException catch (error, stackTrace) {
  // Handle CustomException...
}
on Exception catch (error, stackTrace) {
  // Handle any other unknown exceptions...
}

This way you can handle specific exceptions explicitly, but you also don't miss generic exceptions that you didn't expect. Let me know what you think and thanks for bringing this up! 🙏

PlugFox commented 3 years ago

@felangel yes, not right now, i meaning when it arrived in stable channel

narcodico commented 3 years ago

I'm sure the examples were meant to showcase a minimal approach to error handling, a subjective topic. I for example prefer to pass the original stack trace as part of my custom exception class; that way when logged with tools like crashlytics you can have access to both the in-app stack trace as well as the original stack trace.

PlugFox commented 3 years ago

@narcodico yeah, wrapper for original exception is good workaround.

Nowdays this is due to a dart design error. And with very old issue https://github.com/dart-lang/sdk/issues/10297 since 2013.

Also, if you use dart code metric for additional useful rules, you would like my issue, which was taken to work https://github.com/dart-code-checker/dart-code-metrics/issues/556

Since from //@dart=2.16 that problem will go away with Error.throwWithStackTrace

PS: good library for managering stack trace: stack_trace

narcodico commented 3 years ago

Since from //@dart=2.16 that problem will go away with Error.throwWithStackTrace

That would work for certain situations. I prefer to throw custom exceptions since by definition an error implies something unexpected while when catching something we expect we're dealing with exceptions rather than errors.