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

Escape from uncaught exceptions for IDEs #2104

Merged
merged 4 commits into from
Feb 17, 2024
Merged

Conversation

EricssonXD
Copy link
Contributor

@EricssonXD EricssonXD commented Jan 24, 2024

I faced the same issue as #314 and #1869, and I also understand that it is not an error with the dio package but an error with the Flutter SDK / VScode, where VScode does not recognise the exceptions as handled when using Future.catchError().

Despite it not being a problem of this package, I believe "fixing" it such that it avoids the above mentioned problem is still the correct approach, the following modification avoids this problem, and allows the example Error Handling code on pub.dev works with VScode.

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@EricssonXD EricssonXD requested a review from a team as a code owner January 24, 2024 04:12
@AlexV525
Copy link
Member

@EricssonXD Hi, thanks for taking the time to investigate the cause. Very much appreciated!

I was wonder if adding the paired async/await could be addressed by simply adding return to the _dispatchRequest. WDYT?

@AlexV525 AlexV525 changed the title Allow Vscode to recognise Exceptions Escape from uncaught exceptions for IDEs Jan 24, 2024
@AlexV525
Copy link
Member

Could also help to identify if this is relevant fix for #1869?

@EricssonXD
Copy link
Contributor Author

EricssonXD commented Jan 24, 2024

Could also help to identify if this is relevant fix for #1869?

@AlexV525 I forgot to mention it in the description, this is also an issue that I came across which is the exact problem I ran into initially

@EricssonXD
Copy link
Contributor Author

EricssonXD commented Jan 24, 2024

@EricssonXD Hi, thanks for taking the time to investigate the cause. Very much appreciated!

I was wonder if adding the paired async/await could be addressed by simply adding return to the _dispatchRequest. WDYT?

As the function is a parameter that gets passed to the interceptor, which expects a void Function(RequestOptions options, RequestInterceptorHandler handler), it cannot return any value. Unless we also modify the interceptor? Although I wouldn't suggest it as it seems like a lot more work and might lead to other unexpected problems

@EricssonXD
Copy link
Contributor Author

@EricssonXD Hi, thanks for taking the time to investigate the cause. Very much appreciated!

I was wonder if adding the paired async/await could be addressed by simply adding return to the _dispatchRequest. WDYT?

Is there any reasons why you would like to avoid async/await? From what I see in how the code is used, I don't think there should be any difference in terms of performance. But I'm still new to Flutter, so correct me if I'm wrong.

Signed-off-by: Ericsson <[email protected]>
@AlexV525
Copy link
Member

Is there any reasons why you would like to avoid async/await? From what I see in how the code is used, I don't think there should be any difference in terms of performance. But I'm still new to Flutter, so correct me if I'm wrong.

You may spot that we've already implemented .then and .catchError for the method. Theoretically, all exceptions should be caught here. Awaiting a future that is not void is not recommended.

Consider writing like this:
image

@EricssonXD
Copy link
Contributor Author

EricssonXD commented Jan 24, 2024

That's actually what I wrote initially during debugging but I found out adding only the await keyword somehow made it work which lead to my proposed code. I have updated my code now, but I don't quite understand what do you mean by

Awaiting a future that is not void is not recommended.

Isn't this new code also awaiting a future that is not void? Or do you mean like we shouldn't await a value that is not void if we are not using it?

@AlexV525
Copy link
Member

That's actually what I wrote initially during debugging but I found out adding only the await keyword somehow made it work which lead to my proposed code. I have updated my code now, but I don't quite understand what do you mean by

Awaiting a future that is not void is not recommended.

Isn't this new code also awaiting a future that is not void? Or do you mean like we shouldn't await a value that is not void if we are not using it?

What I mean is we shouldn't wait for a future that returns value but not use it eventually.

In general:

Future<String> foo() async {
  await Future(() {});
  return 'bar';
}

void main() async {
  // Good.
  final result = await foo();
  print(result);

  // Bad.
  await foo();
}

@EricssonXD
Copy link
Contributor Author

Ahhh I see, are there any more problems that I should address in my pull request?

@AlexV525 AlexV525 added e: good for newcomers Good for newcomers p: dio Targeting `dio` package s: bug Something isn't working labels Jan 24, 2024
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

One nit:

dio/CHANGELOG.md Outdated Show resolved Hide resolved
@AlexV525
Copy link
Member

I'd consider the request as test exempt, as it doesn't affect any actual behavior. Waiting for other inputs.

@AlexV525
Copy link
Member

AlexV525 commented Feb 7, 2024

@kuhnroyal @ueman Any inputs?

@AlexV525
Copy link
Member

Going to merge this and apply inputs if any in the future.

@AlexV525 AlexV525 merged commit 2b59869 into cfug:main Feb 17, 2024
3 checks passed
@kuhnroyal
Copy link
Member

I think the fix for the root cause is out in Dart:master now. But this looks cleaner in any case.
We should try to avoid then & catchError in all new could just for readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e: good for newcomers Good for newcomers p: dio Targeting `dio` package s: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants