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

feat: support multiple headers #1090

Closed
wants to merge 3 commits into from
Closed

feat: support multiple headers #1090

wants to merge 3 commits into from

Conversation

medz
Copy link

@medz medz commented Dec 22, 2023

  • Thanks for your contribution! Please replace this text with a description of what this PR is changing or adding and why, list any relevant issues, and review the contribution guidelines below.

  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@brianquinlan
Copy link
Collaborator

Hi @medz, is this ready for review?

@medz
Copy link
Author

medz commented Jan 3, 2024

@brianquinlan I'm ready, but there are a few problems I can't solve. I observed that the mock client uses isolate, and my headers implementation cannot send messages. I don't have a good solution for the time being.

@@ -157,7 +166,8 @@ Future<String> read(Uri url, {Map<String, String>? headers}) =>
///
/// For more fine-grained control over the request and response, use [Request]
/// instead.
Future<Uint8List> readBytes(Uri url, {Map<String, String>? headers}) =>
Future<Uint8List> readBytes(Uri url,
Copy link
Author

Choose a reason for hiding this comment

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

In addition, similar to here, I think it is completely possible to use Object? to set headers, because headers are made according to MDN standards, so in theory it can allow many structures to be converted into headers

@brianquinlan
Copy link
Collaborator

You are changing the public interface to several widely-used interfaces, which would be massively breaking.

Multiple header values are supported by separating them my commas and assigning them to the name field e.g.

headers = {"fruit": "apple,banana"};

Maybe it would be more practical to add a function to split and combine comma-separated field values e.g.

List<String> splitHeaderValues(String values);
joinHeaderValues(Iterable<String> values);

...but now that I think about it, those functions would be trivial.

Maybe the best solution would be to document? Like at https://pub.dev/documentation/http/latest/http/BaseRequest/headers.html

@medz
Copy link
Author

medz commented Jan 4, 2024

You are changing the public interface to several widely-used interfaces, which would be massively breaking.

Multiple header values are supported by separating them my commas and assigning them to the name field e.g.

headers = {"fruit": "apple,banana"};

Maybe it would be more practical to add a function to split and combine comma-separated field values e.g.

List<String> splitHeaderValues(String values);
joinHeaderValues(Iterable<String> values);

...but now that I think about it, those functions would be trivial.

Maybe the best solution would be to document? Like at https://pub.dev/documentation/http/latest/http/BaseRequest/headers.html

This PR will indeed cause breaking changes, but it will make subsequent releases permanent. Splitting using , is indeed possible, but many http header values contain , which can easily cause wrong splitting.

Maybe changing the type of e.g. http.post(headers: xxx) to Object? would be a good choice, and content built with previous versions can be migrated to the new headers implementation without changing the value type. I will make a commit later to implement it

@medz
Copy link
Author

medz commented Jan 4, 2024

The PR close, new #1106 replace this

@medz medz closed this Jan 4, 2024
@medz medz deleted the headers branch January 4, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants