feat(Http): Http service can make cross-site requests (get, post, put, e... #1026

mvuksano · 2014-05-09T11:23:47Z

...tc.) which use credentials (such as cookies or authorization headers).

mvuksano · 2014-05-09T11:26:43Z

test/core_dom/http_spec.dart

+        }));
+
+        it('head should pass on withCredentials to backend', async((FakeBackend backend) {
+          http.head('/url', withCredentials: true);


@jbdeboer I'm not sure if all these tests are needed. They all test pretty much the same thing. I'm thinking of keeping just the first test. What are your thoughts?

maybe a single test with

['get', 'put', 'delete', 'head', ...].forEach((method) { Function.apply(...); expect(backend.method).toEqual(method); });

@vicb Some parameters are a bit different for those methods. Post and Put have data parameter mandatory, while others only require url. I thought about this approach but I'm not sure if there's enough value that those tests bring...

mhevery · 2014-05-09T20:44:19Z

lib/core_dom/http.dart

@@ -422,6 +422,7 @@ class Http {
    data,
    Map<String, dynamic> params,
    Map<String, dynamic> headers,
+    withCredentials: false,


jbdeboer · 2014-05-15T22:21:40Z

test/core_dom/http_spec.dart

@@ -1356,3 +1373,40 @@ class FakeFile implements File {
  Blob slice([int start, int end, String contentType]) => null;
  int get lastModified => new DateTime.now().millisecondsSinceEpoch;
 }
+
+class FakeBackend extends Mock implements HttpBackend {


Why not use MockHttpBackend?

@jbdeboer MockHttpBackend won't tell me if the method was called with correct parameters. Is there a way to get that info from MockHttpBacked?

could you use something like https://github.com/angular/angular.dart/blob/a5d5c2b17a64acadb7935bf734da7e92d3a4cf74/test/core_dom/http_spec.dart#L72

It should be extended to support withCrdentials, ie
backend.expect('GET', '/url', withCrdentials: true).respond('');

+1 to @vicb's comment. You should extend the MockHttpBackend as well.

mvuksano · 2014-05-18T05:53:03Z

@vicb @jbdeboer I've refactored this to use MockHttpBackend (and added support for withCredentials to it). I noticed one thing - the positional optional parameters are not that nice to work with. I think it might be beneficial to use named optional parameters when setting up expect and when. In my opinon it would make tests more readable and would make expectations/whens a lot more explicit.

Instead of having

hb.when('GET', '/url1', [], [], true).respond(201, 'another', {});

It'd be nicer and more readable to have

hb.when('/url1', method: 'GET', data: [], headers: [], withCredentials: true).respond(201, 'another', {});

This way http backend configuration is a lot more specific and readable. This would also make HttpBackend, in a way, aligned with HttpRequest which also uses optional named parameters.

Do you guys think it's worth refactoring that MockHttpBackend to use optional named args instead of optional positional args?

vicb · 2014-05-22T14:13:34Z

lib/mock/http_backend.dart

@@ -102,6 +104,10 @@ class MockHttpExpectation {
    return JSON.encode(data) == JSON.encode(d);
  }

+  bool matchWithCredentials(withCredentials) {


what about bool matchWithCredentials(bool value) => withCredentials == value

…, etc.) which use credentials (such as cookies or authorization headers). Closes dart-archive#945

vicb · 2014-05-22T14:16:22Z

lib/mock/http_backend.dart

+      if (!expectation.matchWithCredentials(withCredentials))
+        throw ['Expected $expectation with different withCredentials\n'
+        'EXPECTED: ${prettyPrint(expectation.withCredentials)}\n'
+        'GOT:      ${prettyPrint(withCredentials)}'];


I would align strings here (continuation lines should be indented w/ at least 4 spaces).

Side question: why do we throw lists instead of throwing strings ?

vicb · 2014-05-22T14:22:58Z

👍 for named parameters

MockHttpBackend takes one more parameter, withCredentials. This is used to mimic withCredentials parameter from real HttpRequest.

…, etc.) which use credentials (such as cookies or authorization headers). Closes #945 Closes #1026

jbdeboer · 2014-05-22T21:32:32Z

I reworked MockHttpBackend.call to take named parameters. Other than that, LGTM. This is in the presubmit queue now.

…, etc.) which use credentials (such as cookies or authorization headers). Closes #945 Closes #1026

…, etc.) which use credentials (such as cookies or authorization headers). Closes dart-archive#945 Closes dart-archive#1026

mvuksano reviewed May 9, 2014
View reviewed changes

mvuksano added cla: yes and removed cla: no labels May 9, 2014

mhevery reviewed May 9, 2014
View reviewed changes

mhevery added the ReadyToMerge label May 9, 2014

jbdeboer reviewed May 15, 2014
View reviewed changes

jbdeboer removed the ReadyToMerge label May 16, 2014

vicb reviewed May 22, 2014
View reviewed changes

feat(Http): Http service can make cross-site requests (get, post, put…

9460160

…, etc.) which use credentials (such as cookies or authorization headers). Closes dart-archive#945

vicb reviewed May 22, 2014
View reviewed changes

feat(MockHttpBackend): Add support for withCredentials.

92706b5

MockHttpBackend takes one more parameter, withCredentials. This is used to mimic withCredentials parameter from real HttpRequest.

mvuksano added a commit that referenced this pull request May 22, 2014

feat(Http): Http service can make cross-site requests (get, post, put…

3036fe0

…, etc.) which use credentials (such as cookies or authorization headers). Closes #945 Closes #1026

mvuksano closed this in 3ef9d8e May 22, 2014

mvuksano added a commit that referenced this pull request May 30, 2014

feat(Http): Http service can make cross-site requests (get, post, put…

eac42c1

…, etc.) which use credentials (such as cookies or authorization headers). Closes #945 Closes #1026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Http): Http service can make cross-site requests (get, post, put, e... #1026

feat(Http): Http service can make cross-site requests (get, post, put, e... #1026

mvuksano commented May 9, 2014

mvuksano May 9, 2014

vicb May 9, 2014

mvuksano May 9, 2014

mhevery May 9, 2014

jbdeboer May 15, 2014

mvuksano May 15, 2014

vicb May 16, 2014

jbdeboer May 16, 2014

mvuksano commented May 18, 2014

vicb May 22, 2014

mvuksano May 22, 2014

vicb May 22, 2014

vicb commented May 22, 2014

jbdeboer commented May 22, 2014

feat(Http): Http service can make cross-site requests (get, post, put, e... #1026

feat(Http): Http service can make cross-site requests (get, post, put, e... #1026

Conversation

mvuksano commented May 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvuksano commented May 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented May 22, 2014

jbdeboer commented May 22, 2014