-
Notifications
You must be signed in to change notification settings - Fork 366
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 for Request set bodyFields, need to be able to have List<String> … #49
Conversation
Fix for #47 |
lib/src/request.dart
Outdated
@@ -113,10 +113,10 @@ class Request extends BaseRequest { | |||
'content-type "application/x-www-form-urlencoded".'); | |||
} | |||
|
|||
return Uri.splitQueryString(body, encoding: encoding); | |||
return splitQueryString(body, encoding: encoding); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, returning a List<String>
value here will break backwards-compatibility, as will changing the return type to Map<String, Object>
. I'm also not sure that returning a map that may have list or string keys is the right API in the abstract; it would make it easy for code written assuming strings to crash or, worse, behave unpredictably if the browser caused a list to be returned instead.
I think the ideal solution here would be to have a sort of multimap class that implements Map<String, String>
but allows []=
to take a list and has a .getMultiple()
method so users can opt in to getting multiple values. That sort of thing would also be really useful for headers. If you'd like to implement that (maybe in the collection
package), it would be awesome; if you don't want to, maybe we should just make this feature write-only for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with your assessment, want evolved behavior rather than breaking behavior. Multi-map would solve problems in other areas as well. I'll need to bring myself up to speed on lower level details of Dart language and see if I can tackle this. I do want to try, and will let you know if I'm up to the challenge. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is example code of what a MultiMap would look like (if I coded it)
https://github.com/rwrozelle/multimap
Is this what you are looking for? I've played around with using it in local copies of the http and http_server package and it definitely plays nicely with only minimal updates to existing code and backward compatible. I decided on using a parent/child component template because it was the least amount of work and I didn't heavily research the existing Mixin classes, so might need to add some "with" stuff on the class definition?
I'm having a heck of a time compiling the SDK, I need to build a new VM on my laptop to get everything lined up and don't have the time until next year, so I can't put together example of code integrated into Collections package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is example code of what a MultiMap would look like (if I coded it)
https://github.com/rwrozelle/multimapIs this what you are looking for? I've played around with using it in local copies of the http and http_server package and it definitely plays nicely with only minimal updates to existing code and backward compatible. I decided on using a parent/child component template because it was the least amount of work and I didn't heavily research the existing Mixin classes, so might need to add some "with" stuff on the class definition?
The behavior here looks more or less right. We can iterate on the details of the design in a separate CL.
I'm having a heck of a time compiling the SDK, I need to build a new VM on my laptop to get everything lined up and don't have the time until next year, so I can't put together example of code integrated into Collections package.
Good news: you don't have to add it to the SDK! The collection
pub package is the place for new collection types these days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar with term: CL
I've forked the collection package and added my code, do I perform a pull request and reference the original issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not familiar with term: CL
Sorry, that's a Google-ism for "pull request".
I've forked the collection package and added my code, do I perform a pull request and reference the original issues?
Yes please!
…for a map value: #47 Using new MultiMap
# Conflicts: # lib/src/utils.dart
This pull request is dependent upon dart-archive/collection#47 being accepted. |
dart-archive/collection#47 stalled out. Closing this for now. |
…for a map value: #47