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

Throw on invalid platform return type #62

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

krocard
Copy link
Contributor

@krocard krocard commented Oct 4, 2023

Problem (PFL-69)

If the type returned from the platform side is not the expected one, invokeMethod returns null. We are handling it by returning default values.

Changes

As this should never happen, and if it was to, it would be a bug, throw an exception instead of hiding the issue.

Checklist

  • 🗒 CHANGELOG entry if applicable

If the type returned from the platform side is not the expected one,
`invokeMethod returns null`. We are handling it by returning default values.

As this should never happen, and if it was to, it would be a bug,
throw an exception instead of hiding the issue.

PFL-69
@krocard krocard self-assigned this Oct 4, 2023
@krocard krocard requested a review from hawk23 October 4, 2023 13:36
@@ -143,7 +154,7 @@ class Player with PlayerEventHandler implements PlayerApi {
]) async {
final result = await _initializationResult;
if (!result) {
return Future.error('Error initializing player on native platform side.');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapping in Future.error is unnecessary in an async function, it is equivalent to throw.
Additionally, the code was throwing a string when Dart recommends to only throw subclasses of Exception or Error.

Copy link
Collaborator

@hawk23 hawk23 left a comment

Choose a reason for hiding this comment

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

Nice improvement! 💪

During manual testing, I found a lot of those "Invalid type" exceptions in the console. A quick check revealed that now all calls of _invokeMethod<void>() fail with an exception now. I tested this on iOS but it should also fail on Android.

Also, flutter test integration_test fails now (we really should execute those in the CI soon 😅).

Some failing examples from player.dart

@override
Future<void> play() async => _invokeMethod<void>(Methods.play);

@override
Future<void> mute() async => _invokeMethod<void>(Methods.mute);

@override
Future<void> unmute() async => _invokeMethod<void>(Methods.unmute);

@override
Future<void> pause() async => _invokeMethod<void>(Methods.pause);

]) async {
final result = await _invokeMethodNullable<T>(methodName, data);
if (result == null) {
throw Exception('Invalid type returned from the native platform side.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's also print the method name for better debugging:

Suggested change
throw Exception('Invalid type returned from the native platform side.');
throw Exception('Invalid type returned for $methodName from the native platform side.');

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.

Done

`void` is the only named type that is always nullable.
aka `T == T?` for `T = void`.
@krocard
Copy link
Contributor Author

krocard commented Oct 5, 2023

During manual testing, I found a lot of those "Invalid type" exceptions in the console. A quick check revealed that now all calls of _invokeMethod() fail with an exception now. I tested this on iOS but it should also fail on Android.

Good catch. I was expecting the app to crash if any error was raised. I did not realized they would be logged. The issue was due to void being the only type for which T == T?. The problem should be fixed now.

Also, flutter test integration_test fails now (we really should execute those in the CI soon 😅).

I have not been able to run this the test locally. I guess some weird error:

$ flutter test integration_test
00:00 +0: loading /home/krocard/dev/bitmovin/bitmovin-player-flutter/example/integration_test/seeking_test.dart                                                                                                                        R../player_testing/lib/env/env.dart:3:6: Error: Error when reading '../player_testing/lib/env/env.g.dart': No such file or directory
part 'env.g.dart';
     ^
../player_testing/lib/env/env.dart:3:6: Error: Can't use '../player_testing/lib/env/env.g.dart' as a part, because it has no 'part of' declaration.
part 'env.g.dart';
     ^
../player_testing/lib/env/env.dart:14:50: Error: Undefined name '_Env'.
  static const String bitmovinPlayerLicenseKey = _Env.bitmovinPlayerLicenseKey;

But it does have a part of:

$ head -n3 ../player_testing/lib/env/env.dart
import 'package:envied/envied.dart';

part 'env.g.dart';

@hawk23
Copy link
Collaborator

hawk23 commented Oct 5, 2023

I have not been able to run this the test locally. I guess some weird error:

Please have look at Integration Tests section in CONTRIBUTING.md. You need to create the .env file and run dart run build_runner build once to solve this.

@krocard
Copy link
Contributor Author

krocard commented Oct 5, 2023

I solved it by running dart run build_runner build in player_testing. Running it only in the root did not generate env.g.dart in player_testing (it did everywhere else but there).
I have no idea why. I even started a clean repo without observing differences.

@krocard krocard merged commit e2926d1 into main Oct 5, 2023
3 checks passed
@krocard krocard deleted the PFL-69/throw-on-invalid-platform-return-type branch October 5, 2023 16:02
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.

2 participants