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

Conditionally allows relative path for baseUrl #2087

Closed
mitryp opened this issue Jan 8, 2024 · 11 comments · Fixed by #2094
Closed

Conditionally allows relative path for baseUrl #2087

mitryp opened this issue Jan 8, 2024 · 11 comments · Fixed by #2094
Labels
fixed p: dio Targeting `dio` package platform: web s: feature This issue indicates a feature request

Comments

@mitryp
Copy link

mitryp commented Jan 8, 2024

Package

dio

Version

5.4.0

Operating-System

Web

Output of flutter doctor -v

No response

Dart Version

3.1.5

Steps to Reproduce

Run the following snippet with dart run:

void main() {
  final dio = Dio(BaseOptions(baseUrl: 'api/'));
}

Expected Result

A new Dio instance is created with the given baseUrl and can be used as usual.
It is a normal situation for web apps to separate their API under a separate endpoint group: https://example.com/api/....

Actual Result

The code above produces the following Assertion Error:

Unhandled exception:
'package:dio/src/options.dart': Failed assertion: line 145 pos 16: 'baseUrl.isEmpty || Uri.parse(baseUrl).host.isNotEmpty': is not true.
#0      _AssertionError._doThrowNew (dart:core-patch/errors_patch.dart:51:61)
#1      _AssertionError._throwNew (dart:core-patch/errors_patch.dart:40:5)
#2      new BaseOptions (package:dio/src/options.dart:145:16)
#3      main (package:anthill_front/src/shared/utils/http_client.dart:8:19)
#4      _delayEntrypointInvocation.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:296:19)
#5      _RawReceivePort._handleMessage (dart:isolate-patch/isolate_patch.dart:189:12)

Process finished with exit code 255

The exception can be avoided if one changes the baseUrl in the BaseOptions after it was created:

final dio = Dio(BaseOptions()..baseUrl = 'api/');

Which is quite strange and not expected solution.

@mitryp mitryp added h: need triage This issue needs to be categorized s: bug Something isn't working labels Jan 8, 2024
@AlexV525
Copy link
Member

AlexV525 commented Jan 8, 2024

You should write https://example.com/api/ for baseUrl.

@mitryp
Copy link
Author

mitryp commented Jan 8, 2024

You should write https://example.com/api/ for baseUrl.

The frontend does not necessarily know the full path.
It's not clear to me, why using a relative path isn't allowed.

@AlexV525
Copy link
Member

AlexV525 commented Jan 9, 2024

How do the instance be able to make requests if the full path is unavailable?

@mitryp
Copy link
Author

mitryp commented Jan 9, 2024

Web applications can request the same origin using relative paths.
Take a look at a W3Schools reference
They also suggest to use relative paths when possible

@AlexV525
Copy link
Member

AlexV525 commented Jan 9, 2024

Dio won't know your origin unless you define so. It's not a Web application.

Closing as intended and won't fix.

@AlexV525 AlexV525 closed this as not planned Won't fix, can't repro, duplicate, stale Jan 9, 2024
@AlexV525 AlexV525 added i: wontfix This will not be worked on and removed h: need triage This issue needs to be categorized s: bug Something isn't working labels Jan 9, 2024
@kuhnroyal
Copy link
Member

Not so sure, this may actually be a valid request for Flutter Web platform.

@mitryp
Copy link
Author

mitryp commented Jan 9, 2024

The problem is not that Dio doesn't know something, but it just stops users from using relative paths with an assert.

You cannot as that it is not a web app, because Dio can indeed be used as a client in a web application.

As for me, it's quite strange to forbid the usage of something that is a part of the specification for no reason, especially when the actual request functionality works and the problem is in an unnecessary assert statement.

Or is there a reason why it is forbidden?

@AlexV525
Copy link
Member

AlexV525 commented Jan 9, 2024

Not so sure, this may actually be a valid request for Flutter Web platform.

It could be possible to resolve by Uri.base.resolve(path) specifically on Web.
https://api.flutter.dev/flutter/dart-core/Uri/base.html

However, it's also available when you're setting the baseUrl.

@mitryp
Copy link
Author

mitryp commented Jan 9, 2024

Yes, if you look at the initial issue, it is possible to bypass the assert using the setter for baseUrl after the BaseOptions is created.

But if it's so, why the asset is there in the first place?

@AlexV525
Copy link
Member

AlexV525 commented Jan 9, 2024

You cannot as that it is not a web app, because Dio can indeed be used as a client in a web application.

I have the opposite opinion here. Dio is not in any form, an application. As you declared, it can only be used in a Web application.

As for me, it's quite strange to forbid the usage of something that is a part of the specification for no reason, especially when the actual request functionality works and the problem is in an unnecessary assert statement.

Could you provide a reproducible example that it can run without the assertion?

But if it's so, why the asset is there in the first place?

You can use the blame view to determine when it's been introduced to find out the cause.

@mitryp
Copy link
Author

mitryp commented Jan 9, 2024

Could you provide a reproducible example that it can run without the assertion?

I created a repo with the minimal reproducible example of both the exception and the supposed behaviour. Its README describes the steps to reproduce both situations.

You can use the blame view to determine when it's been introduced to find out the cause.

diox/#18 has an issue of #1373 connected, but it's still unclear why baseUrl must include the host in the first place.

@AlexV525 AlexV525 reopened this Jan 9, 2024
@AlexV525 AlexV525 changed the title AssertionError when using a relative baseUrl in BaseOptions Conditionally allows relative path for baseUrl Jan 9, 2024
@AlexV525 AlexV525 added s: feature This issue indicates a feature request p: dio Targeting `dio` package platform: web and removed i: wontfix This will not be worked on labels Jan 9, 2024
AlexV525 added a commit that referenced this issue Jan 12, 2024
Resolves #2087.

### New Pull Request Checklist

- [x] I have read the
[Documentation](https://pub.dev/documentation/dio/latest/)
- [x] I have searched for a similar pull request in the
[project](https://github.com/cfug/dio/pulls) and found none
- [x] I have updated this branch with the latest `main` branch to avoid
conflicts (via merge from master or rebase)
- [x] I have added the required tests to prove the fix/feature I'm
adding
- [x] I have updated the documentation (if necessary)
- [x] I have run the tests without failures
- [x] I have updated the `CHANGELOG.md` in the corresponding package
@AlexV525 AlexV525 added the fixed label Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed p: dio Targeting `dio` package platform: web s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants