Updating request library to mimic fetch API #261

rorticus · 2017-01-06T19:50:49Z

This PR updates the request APIs to mimic the fetch APIs, which makes it possible to do cool stuff like look at the response headers before the body is downloaded.

This also introduces an event based Response object that can be used to monitor the download progress and send responses to streams (a helper utility for that will be provided in dojo-streams).

Issue: #253

codecov-io · 2017-01-09T14:50:15Z

Codecov Report

Merging #261 into master will decrease coverage by -1.64%.

@@           Coverage Diff            @@
##           master   #261      +/-   ##
========================================
- Coverage   95.63%    94%   -1.64%     
========================================
  Files          34     37       +3     
  Lines        1900   2167     +267     
  Branches      373    410      +37     
========================================
+ Hits         1817   2037     +220     
- Misses         25     52      +27     
- Partials       58     78      +20

Impacted Files	Coverage Δ
src/text.ts	`82.75% <100%> (+0.3%)`	✅
src/has.ts	`100% <100%> (ø)`	✅
src/QueuingEvented.ts	`100% <100%> (ø)`
src/request/util.ts	`66.66% <33.33%> (-22.23%)`	❌
src/request/Headers.ts	`76.92% <76.92%> (ø)`
src/request/Response.ts	`81.81% <81.81%> (ø)`
src/request/TimeoutError.ts	`85.71% <85.71%> (ø)`
src/request/providers/xhr.ts	`88.27% <88.27%> (ø)`
src/request/providers/node.ts	`89.1% <89.1%> (ø)`
src/request/ProviderRegistry.ts	`92.85% <92.85%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4793c9...724b046. Read the comment docs.

rishson · 2017-01-11T13:21:49Z

src/request.ts


-		return super.register(entryTest, value, first);
+const request: {


Any reason why we don't also support OPTIONS and HEAD? At the moment we support a list that is neither the CORS safelist of verbs or the verbs subject to normalization in the fetch spec.

Not a good reason that I know of 😄 You can still still make a custom request with whatever method you want, but adding OPTIONS and HEAD should be fairly trivial, so we can do that too!

Yeah, given we actually normalize, e.g. send POST for a post(), then probably best we support all the verbs that spec recommends normalization for. As you say, nothing to stop them doing a custom request for post or pOsT..

Original reason was likely based on what was done in Dojo 1, fwiw. :)

dylans

I think I'm ok with this... should have others review as there's a good risk of it causing breaking changes.

agubler · 2017-01-31T15:28:55Z

I don't think we have anything dojo 2 using request, so i'm also okay with the changes. See you've asked for a review from @bryanforbes, that's probably a good idea 👍

…treams

bryanforbes · 2017-02-09T16:34:17Z

src/request/providers/node.ts

+	}
+
+	const { limit: redirectLimit = DEFAULT_REDIRECT_LIMIT } = options.redirectOptions;
+	const { count: redirectCount = 0 } = options.redirectOptions;


Would it be clearer and/or more efficient to format these two lines as such?

const { limit: redirectLimit = DEFAULT_REDIRECT_LIMIT, count: redirectCount = 0 } = options.redirectOptions;

bryanforbes · 2017-02-09T16:36:08Z

src/request/node.ts

@@ -1,334 +0,0 @@
-import Task from '../async/Task';


In order for git to recognize the move of this file, it should be moved in one commit and changed in another. I would recommend rewriting history in this branch to add a commit before starting that moves files into place before making changes.

So.. I played around with a few ways to get git to recognize this as a rename rather than a new file. I think that it worked.. but it dawned on me that since we only squash from github, that special 'renaming commit' will get squashed into the other commits and we'd lose that history anyways, ending up with a single "'delete xyz', 'add xyy'" flow with files so different that git won't think they are a rename (the problem we have now).

Oh right. Then you can safely ignore me.

bryanforbes · 2017-02-09T16:49:10Z

src/request/providers/node.ts

+				response.status < 400
+			) {
+				const redirectOptions = options.redirectOptions || {};
+				const newOptions = Object.create(options);


This creates a shallow copy of options, so assigning to a sub-object (as is being done on line 227 above) could potentially modify the original. Since no such function exists in @dojo/core to do a deep create, we should create a PR for one and use it here.

would we not just use deepAssign({}, options)?

I'll go with that @agubler

deepAssign({}, options) would also work. I figured there was a reason the code was using Object.create() instead of Object.assign(), so to keep the code somewhat the same, I was noting that we don't have the Object.create() counterpart in @dojo/lang. Once you start iterating over properties, you may as well use deepAssign() anyway.

bryanforbes · 2017-02-09T17:02:59Z

src/request/providers/node.ts

+
+		const sanitizedUrl = urlUtil.format(parsedUrl);
+
+		error.message = '[' + requestOptions.method + ' ' + sanitizedUrl + '] ' + error.message;


Are there cases where setting error.message would throw?

Maybe, but either way we're throwing an error. I could swallow errors to error.message and just ignore it if it didn't work? What do you think?

I'm really not sure what the best practice is here. At this point, it might be best to just leave this and if we hit this in reality (rather than trying to cover every potential case), we can adjust.

OK. Let's ignore this for now.

bryanforbes · 2017-02-09T17:05:56Z

src/request/providers/xhr.ts

+function noop () {}
+
+function setOnError(request: XMLHttpRequest, reject: Function) {
+	request.onerror = function (event) {


Should this use addEventListener()?

bryanforbes · 2017-02-09T17:07:41Z

src/request/providers/xhr.ts

+		options.method = 'GET';
+	}
+
+	if ((!options.user || !options.password) && options.auth) {


What is the reasoning behind allowing two ways to pass authentication information?

bryanforbes · 2017-02-09T17:13:41Z

src/request/providers/xhr.ts

+				const task = new Task<XMLHttpRequest>((resolve, reject) => {
+					timeoutReject = reject;
+
+					request.onprogress = function (event: any) {


This only handles download progress. It would be nice (perhaps in another PR) to handle upload progress as well.

bryanforbes · 2017-02-09T17:14:28Z

src/request/providers/xhr.ts

+	request.open(options.method, requestUrl, !options.blockMainThread, options.user, options.password);
+
+	if (has('filereader') && has('blob')) {
+		request.responseType = 'blob';


Does this need to be set before calling request.open()?

This has been addressed!

bryanforbes · 2017-02-09T17:32:42Z

src/QueuingEvented.ts

@@ -0,0 +1,67 @@
+import Evented from './Evented';


Please add some documentation to this class about what it does.

bryanforbes

👍

rishson reviewed Jan 11, 2017

View reviewed changes

dylans added this to the 2017.01 milestone Jan 12, 2017

rorticus force-pushed the new-requests branch from d254b27 to f1846e5 Compare January 31, 2017 14:00

dylans approved these changes Jan 31, 2017

View reviewed changes

rorticus requested a review from bryanforbes January 31, 2017 15:17

rorticus mentioned this pull request Jan 31, 2017

Core 252 dojo/streams#6

Merged

3 tasks

dylans modified the milestones: 2017.01, 2017.02 Jan 31, 2017

dylans mentioned this pull request Feb 1, 2017

Add REST store support dojo/stores#48

Closed

7 tasks

rorticus force-pushed the new-requests branch from 3590da9 to 3052c3f Compare February 6, 2017 13:04

This was referenced Feb 6, 2017

core/request : node provider only redirects URLs with protocol and host #278

Closed

core/request : node provider does not decompress #259

Closed

rorticus and others added 11 commits February 9, 2017 10:55

Updating request library to mimic fetch API, removing dependency on s…

90a389e

…treams

Exporting correct interface file now

f494bb0

Request respones now using a queing event model

728d09e

Removing arraybuffer/blob tests if the environment doesnt support them

d183378

Removing arrayBuffer test if blob isnt supported

dbca8b4

Adding OPTIONS, HEAD convienence methods

78cfb1b

Fixing import

f3845a3

Allowing for relative redirects in node provider, issue dojo#278

7827d27

Rebasing onto latest

b5faa1a

Handling gzip/deflate compression in node provider issue dojo#259

25bc77f

Rebasing on latest

1b17667

rorticus force-pushed the new-requests branch from 647cac2 to 1b17667 Compare February 9, 2017 15:58

bryanforbes reviewed Feb 9, 2017

View reviewed changes

Review feedback

d1c5fec

Moving XHR responseType back

724b046

rorticus mentioned this pull request Feb 10, 2017

Add upload progress reporting #285

Closed

bryanforbes approved these changes Feb 10, 2017

View reviewed changes

rorticus merged commit 5d21060 into dojo:master Feb 10, 2017

rorticus mentioned this pull request Feb 10, 2017

Simplify core/request #253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating request library to mimic fetch API #261

Updating request library to mimic fetch API #261

rorticus commented Jan 6, 2017 •

edited

Loading

codecov-io commented Jan 9, 2017 •

edited

Loading

rishson Jan 11, 2017 •

edited

Loading

rorticus Jan 11, 2017

rishson Jan 11, 2017 •

edited

Loading

rorticus Jan 11, 2017

rishson Jan 11, 2017

dylans Jan 11, 2017

dylans left a comment

agubler commented Jan 31, 2017

bryanforbes Feb 9, 2017

rorticus Feb 9, 2017

bryanforbes Feb 9, 2017

rorticus Feb 10, 2017

bryanforbes Feb 10, 2017

bryanforbes Feb 9, 2017

agubler Feb 9, 2017

rorticus Feb 9, 2017

bryanforbes Feb 9, 2017

bryanforbes Feb 9, 2017

rorticus Feb 9, 2017

bryanforbes Feb 9, 2017

rorticus Feb 9, 2017

bryanforbes Feb 9, 2017

bryanforbes Feb 9, 2017

bryanforbes Feb 9, 2017

rorticus Feb 10, 2017

bryanforbes Feb 9, 2017

rorticus Feb 10, 2017

bryanforbes Feb 9, 2017

rorticus Feb 9, 2017

bryanforbes left a comment


		return super.register(entryTest, value, first);
		const request: {


		const sanitizedUrl = urlUtil.format(parsedUrl);

		error.message = '[' + requestOptions.method + ' ' + sanitizedUrl + '] ' + error.message;

Updating request library to mimic fetch API #261

Updating request library to mimic fetch API #261

Conversation

rorticus commented Jan 6, 2017 • edited Loading

codecov-io commented Jan 9, 2017 • edited Loading

Codecov Report

rishson Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishson Jan 11, 2017 • edited Loading

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

dylans left a comment

Choose a reason for hiding this comment

agubler commented Jan 31, 2017

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

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

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

bryanforbes left a comment

Choose a reason for hiding this comment

rorticus commented Jan 6, 2017 •

edited

Loading

codecov-io commented Jan 9, 2017 •

edited

Loading

rishson Jan 11, 2017 •

edited

Loading

rishson Jan 11, 2017 •

edited

Loading