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

To save typing rename HttpRequest#queryParameters to HttpRequest#params #2641

Closed
DartBot opened this issue Apr 18, 2012 · 18 comments
Closed
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io
Milestone

Comments

@DartBot
Copy link

DartBot commented Apr 18, 2012

This issue was originally filed by @MarkBennett


What steps will reproduce the problem?

  1. Edit a Dart file in a plain text editor
  2. Type queryParameters.
  3. Realize you just wasted 9 keystrokes.

What is the expected output? What do you see instead?

    I would expect to only need to type params, but instead had to type queryParameters. I would expect details of this function to be included in the source code documentation, and for the function name to be more concise.

What version of the product are you using? On what operating system?

    Version 0.1.0.201204121423, Build 6479
    Dart SDK version 6478, Dartium version

Please provide any additional information below.

    As an enhancement to this bug, consider displaying function documentation in the autocomplete in the Dart Editory.

@madsager
Copy link
Contributor

cc @sgjesse.
Added Area-IO, Triaged labels.

@sgjesse
Copy link
Contributor

sgjesse commented Apr 19, 2012

Having short and simple and descriptive names is definitely one of design criteria for the dart:io APIs.

In relation to queryParameters the reason for the long name is that we need to support handling of parameters from form posts as well as from the query string. Handling of POSTs with content type application/x-www-form-urlencoded will be supported soon (we will have to see about multipart/form-data - especially if file uploads are involved)‎, and to distinguish these parameters from the query string using getter formParameters was intended.

I agree that both are long names, so maybe "query" and "form" are better names.

Alternatively we could use "params" as suggested together with "form".

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @MarkBennett


What about combining POST and GET parameters into a single
HttpRequest#params, but keeping the longer names for accessing the GET and
POST params separately? This is how it's done in Rails and Pylons. I
believe it's also the same in Node but will need to check.

I would really like to focus on keeping the Dart API simple.

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @seaneagan


I would suggest to change HttpRequest#uri from String to Uri, and then remove HttpRequest#{path,queryParameters,queryString}, and then fix issue #2645. Then all uri related information is available in one place:

interface HttpRequest {
  //...
  final Uri uri;
}

Then you can just do:

// see issue #2645
request.uri.query['foo']
request.uri.queryString

request.uri.path

and in the case you need the String uri itself, just do:

request.uri.toString()

or if you are using string interpolation it's even easier...

"request uri is: ${request.uri}"

...since that calls toString implicitly.

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @MarkBennett


I like the idea of combining the query parameters with the uri class, however what about POST parameters? Would they be stored in a separate accessor still?

Perhaps there's an interface which both HttpRequest and uri could implement instead, only the HttpRequest would also include POST params?

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @seaneagan


Yes, Map<String, List<String>> or [List]MultiMap<String, String> would make sense for both query and form parameters. I think since form POSTing is being used less and less all the time due to AJAX etc., it's best to keep the form parameters separate:

interface HttpRequest {
  //...
  final Uri uri;
  final Map<String, List<String>> form;
}

In case someone does want to access params independently of whether they came from a form or the URI, issue #2644 would be useful:

var params = request.form.merge(request.uri.query);
// or vice-versa
var params = request.uri.query.merge(request.form);

...depending on whether you want query or form parameters to take precedence. Not sure if it is common to combine values from a query parameter and form parameter both with the same name, but if so, then a MultiMap#merge would work better.

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @MarkBennett


Can you explain this assertion a bit more as it doesn't match my
experiences at all. Why do you say people are using POSTing less and less?
It's easy to do POST requests from AJAX and I often do them more as they're
an essential part of designing RESTful API's. Generally, the rule I follow
to distinguish between POST and GET methods when designing an API is
simple. POST requests will modify the business data in some meaningful way
(creating new objects, changing existing ones) while GET requests are read
only. There are exceptions such as logging API usage or logging in general
but using POST for changes to the model is a simple way to create safer and
more discoverable APIs.

I like the idea of exposing the form and query parameters seperately, but I
still think there's a case for a single accessor that captures both. This
is how parameters are implemented in Rails, node, and Pylons and has made
writing server code much simpler. In most use cases, a user doesn't care if
a parameter was received from a query string or POST, just that it was
received.

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @seaneagan


I am under the impression that POST parameters only exist when an HTML form is submitted from a web browser (resulting in an application/x-www-form-urlencoded request). And I am asserting that submitting HTML forms is being replaced by ajax and other methods which do not cause a page reload. So I think it is somewhat of a minority use case, which may want to be deprecated at some point, and thus should be separated out.

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @MarkBennett


Ah, ok I think I see the confusion here.

AJAX, or more specifically XHR, allows you to make HTTP requests from
JavaScript code. The underlying requests can use any HTTP method available
to the web browser. This includes HTTP GET, PUT, POST, HEAD and other
methods. Though the page doesn't reload, pages using AJAX calls make
extensive use of HTTP POST requests.

If you'd like to learn more about AJAX or the requests made on a webpage,
an easy way to see it in action is to open up the Developer Tools in Chrome
and go to the network tab. Navigate to a page that makes extensive use of
AJAX (like Gmail.) You may need to reload the page but once you do you'll
see the network tab filling with AJAX requests as you answer and send
email. As I type this message all of the AJAX requests on the screen are
sent via HTTP POST even though the page has not reloaded. This is standard
HTTP traffic for an AJAX application.

Anyway, I think this discussion is taking us off track of this bug. If
you'd like to know more about AJAX I'd be happy to answer your questions on
G+, http://gplus.to/markbennett.

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @seaneagan


Right, POSTs are not declining, but the Content-Type of POSTs is moving towards standard data interchange formats such as "application/json" and away from "application/x-www-form-urlencoded". Thus, code would look more like:

final in = new StringInputStream(request.inputStream));
in.onClosed(() => foo(JSON.parse(in.read())))

as opposed to:

foo(request.form);

@DartBot
Copy link
Author

DartBot commented Apr 19, 2012

This comment was originally written by @MarkBennett


Hmm, submitting the content as JSON is actually something I haven't seen much of as most of the APIs I've written / used still tend to use multipart form encoding for posts.

I could see the advantages of submitting JSON in the request as serializing complex objects as form parameters isn't well defined. What API's are using this technique though?

PS - thanks for the code example, it made what you're saying much clearer. :)

@DartBot
Copy link
Author

DartBot commented Apr 20, 2012

This comment was originally written by ladicek@gmail.com


submitting HTML forms is being replaced by ajax and other methods which do not cause a page reload

Sometimes, but not really always. We need to support both equally well -- let's keep in mind that Dart (not only the language, but also the libraries!) should work well for small and large projects, and moving logic to the client side isn't always the best option.

My personal opinion: we should separate access to query params and form params. Having HttpRequest.params isn't very descriptive, I'd say that it would only make sense for combined query+form params. But renaming "queryParameters" to "queryParams" would be totally OK, even to "query" (it isn't ambiguous). Form params could be accessed by "formParams", but I'm not exactly sure about "form" (but what the heck, it isn't that ambiguous).

@sgjesse
Copy link
Contributor

sgjesse commented Jun 8, 2012

Having the query string as a Uri seems like a good sugestion. As this will be a breaking change setting this as milestone 1.

Handling of FROM POST should be added as well. This part is moved to issue http://code.google.com/p/dart/issues/detail?id=3439.


Added this to the M1 milestone.

@madsager
Copy link
Contributor

madsager commented Sep 3, 2012

Removed this from the M1 milestone.

@sgjesse
Copy link
Contributor

sgjesse commented Apr 16, 2013

Added this to the M5 milestone.

@sgjesse
Copy link
Contributor

sgjesse commented May 30, 2013

Proposed change in https://codereview.chromium.org/15688013/.

This removes the queryParameters getter as the parsed queryParameters are now available from the Uri object.

@sgjesse
Copy link
Contributor

sgjesse commented May 30, 2013

queryParameters getter removed from HttpRequest in https://code.google.com/p/dart/source/detail?r=23413.


Added Fixed label.

@kevmoo
Copy link
Member

kevmoo commented May 14, 2014

Removed Area-IO label.
Added Area-Library, Library-IO labels.

@DartBot DartBot added Type-Defect area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io labels May 14, 2014
@DartBot DartBot added this to the M5 milestone May 14, 2014
dart-bot pushed a commit that referenced this issue Oct 14, 2020
git log --oneline cf9795f3bb209504c349e20501f0b4b8ae31530c..f0c7771b38155d3829a60d60b5dba2784b100811
f0c7771b Set first version with null safety to 2.12 (#2684)
df1140af Warn from get, when mixed mode (#2590)
765778c0 Simplify test detection (#2682)
afd66ea2 Inline the single test asset. (#2681)
059e4796 Simplify the logic for unicode and colors in output (#2679)
35ddaec2 Dartify test tool (#2680)
62f26401 Example for User-Agent (#2678)
e8b4b114 fixes: #2670 pub global activate commands always exit cmd on windows. (#2671)
93e703b1 Improve stack traces in many tests (#2673)
5b540a39 Fix experiments tests for Dart 2.11 (#2672)
b0ac77d8 Bump dependency on pkg:analyzer (#2662)
73f0906e Removed @alwaysThrows (#2642)
88e0a83c Fixed bug in adding dependency to empty dependencies key (#2640)
135d9fa0 Pub add/remove now remove dependencies key if it makes them empty (#2639)
f4cc9673 Fix "pubpsec" typo (#2641)
4686d74d Adding an existing package is now a dataError (#2638)
1e93f47c Checks pubspec keys for potential typos (#2616)
fa5f51ef Vendor yaml_edit (#2633)
ac6d307f Adding a `pub remove` command (#2620)
9d236e00 Adding the `pub add` command (#2618)
04e237f7 Drop upper bound instead of using "any" while resolving in "pub outdated" (#2623)
93954f33 Use InternetAddress.tryParse over try/catch (#2626)
638c81c9 Refine publishing message (#2624)


Allow github


Embed pub as a library into dartdev

Change-Id: Iadc6acb5c3425dfb8848db89820e6c9c8caf16ba
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/167574
Reviewed-by: Jonas Jensen <jonasfj@google.com>
Commit-Queue: Sigurd Meldgaard <sigurdm@google.com>
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io
Projects
None yet
Development

No branches or pull requests

4 participants