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: stacktrace in computed #988

Merged
merged 4 commits into from
Mar 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 64 additions & 9 deletions flutter_mobx/test/flutter_mobx_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,34 @@ void main() {
expect(exception, isInstanceOf<MobXCaughtException>());
});

testWidgets(
'Observer should print full stacktrace of error coming from computed',
(tester) async {
late StackTrace stackTrace;
final errorWrapper = await _testThrowingObserverWithStackTrace(
tester,
firstError: true,
actionThrows: (tester) async {
await tester.pumpWidget(
Observer(
builder: (context) {
Computed(() {
try {
throw Exception();
} on Exception catch (e, st) {
stackTrace = st;
rethrow;
}
}).value;
},
),
);
},
);
expect(errorWrapper.stackTrace, stackTrace);
},
);

testWidgets('Observer unmount should dispose Reaction', (tester) async {
final mock = MockReaction();
when(() => mock.hasObservables).thenReturn(true);
Expand Down Expand Up @@ -345,18 +373,45 @@ Future<MobXCaughtException> _testThrowingObserver(
WidgetTester tester,
Object errorToThrow,
) async {
late Object exception;
return (await _testThrowingObserverWithStackTrace(
tester,
actionThrows: (tester) async {
final count = Observable(0);
await tester.pumpWidget(FlutterErrorThrowingObserver(
errorToThrow: errorToThrow,
builder: (context) => Text(count.value.toString()),
));
count.value++;
},
))
.exception as MobXCaughtException;
}

class _ErrorWrapper {
final Object exception;
final StackTrace? stackTrace;

_ErrorWrapper({required this.exception, required this.stackTrace});
}

Future<_ErrorWrapper> _testThrowingObserverWithStackTrace(
WidgetTester tester, {
required Future<void> Function(WidgetTester tester) actionThrows,
bool firstError = false,
}) async {
_ErrorWrapper? errorWrapper;
final prevOnError = FlutterError.onError;
FlutterError.onError = (details) => exception = details.exception;
FlutterError.onError = (details) {
if (firstError && errorWrapper != null) {
return;
}
errorWrapper =
_ErrorWrapper(exception: details.exception, stackTrace: details.stack);
};

try {
final count = Observable(0);
await tester.pumpWidget(FlutterErrorThrowingObserver(
errorToThrow: errorToThrow,
builder: (context) => Text(count.value.toString()),
));
count.value++;
return exception as MobXCaughtException;
await actionThrows(tester);
return errorWrapper!;
} finally {
FlutterError.onError = prevOnError;
}
Expand Down
4 changes: 4 additions & 0 deletions mobx/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.3.1

- Fix preserving stacktrace in Computed and Reaction when exception thrown inside argument function

## 2.3.0+1

- `pubspec.yaml` updated to include homepage and topics
Expand Down
2 changes: 1 addition & 1 deletion mobx/lib/src/core/computed.dart
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ class Computed<T> extends Atom implements Derivation, ObservableValue<T> {
}

if (_context._hasCaughtException(this)) {
throw _errorValue!;
Error.throwWithStackTrace(_errorValue!, _errorValue!._stackTrace);
}

return _value as T;
Expand Down
13 changes: 8 additions & 5 deletions mobx/lib/src/core/reaction.dart
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class ReactionImpl with DebugCreationStack implements Reaction {
}
Copy link
Member

Choose a reason for hiding this comment

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

Also need to bump up version and add a changelog for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


if (_context._hasCaughtException(this)) {
_reportException(_errorValue!);
_reportException(_errorValue!, _errorValue!.stackTrace);
}

if (notify) {
Expand Down Expand Up @@ -120,7 +120,7 @@ class ReactionImpl with DebugCreationStack implements Reaction {
} on Object catch (e, s) {
// Note: "on Object" accounts for both Error and Exception
_errorValue = MobXCaughtException(e, stackTrace: s);
_reportException(_errorValue!);
_reportException(_errorValue!, s);
}
}

Expand Down Expand Up @@ -165,15 +165,18 @@ class ReactionImpl with DebugCreationStack implements Reaction {
// Not applicable right now
}

void _reportException(Object exception) {
void _reportException(Object exception, StackTrace? stackTrace) {
if (_onError != null) {
_onError!(exception, this);
return;
}

if (_context.config.disableErrorBoundaries == true) {
// ignore: only_throw_errors
throw exception;
if (stackTrace != null) {
Error.throwWithStackTrace(exception, stackTrace);
} else {
throw exception;
}
}

if (_context.isSpyEnabled) {
Expand Down
2 changes: 1 addition & 1 deletion mobx/lib/version.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Generated via set_version.dart. !!!DO NOT MODIFY BY HAND!!!

/// The current version as per `pubspec.yaml`.
const version = '2.3.0+1';
const version = '2.3.1';
2 changes: 1 addition & 1 deletion mobx/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: mobx
version: 2.3.0+1
Copy link
Member

Choose a reason for hiding this comment

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

The set_version command needs to be called from melos to also change the version.dart file for the package

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

formatting is in other pr, because there are several files being changed that are not related to current pr

version: 2.3.1
description: "MobX is a library for reactively managing the state of your applications. Use the power of observables, actions, and reactions to supercharge your Dart and Flutter apps."

repository: https://github.com/mobxjs/mobx.dart
Expand Down
17 changes: 17 additions & 0 deletions mobx/test/exceptions_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,21 @@ void main() {
expect(ex.stackTrace, isNotNull);
}
});

test('should preserve stacktrace', () async {
late StackTrace stackTrace;
try {
Computed(() {
try {
throw Exception();
} on Exception catch (e, st) {
stackTrace = st;
rethrow;
}
}).value;
} on MobXCaughtException catch (e, st) {
expect(st, stackTrace);
expect(st, e.stackTrace);
}
});
}
Loading