Adding fetch implementation of request api, issue #139 #243

rorticus · 2016-11-21T13:46:24Z

Added request/fetch for performing requests based on the fetch API.

agubler · 2016-11-21T14:05:41Z

src/request/fetch.ts

+			timeout && timeout.destroy();
+
+			let { responseType = '' } = options;
+			let body: any = null;


can you type this as a Promise<any> (or not any if you know the body type) and then you wouldn't need the Promise.resolve('body').then... just simply body.then...

Yes! Good point. I guess I had typed that before I realized it was going to have to be a Promise, and then forgot to change it :/

agubler

Just a couple minor comments/questions.

agubler · 2016-11-21T14:06:43Z

src/request/fetch.ts

+					break;
+			}
+
+			let responseHeaders: any = {};


any reason not const?

Also do you know the type of headers, presumably something like maybe

interface Headers { [index string]: string; }

agubler · 2016-11-21T14:08:45Z

src/request/fetch.ts

+
+				case 'xml':
+					body = fetchResponse.text().then((asText: string) => {
+						let parser = new DOMParser();


agubler · 2016-11-21T14:09:25Z

src/request/fetch.ts

+		fetch(request).then((fetchResponse: any) => {
+			timeout && timeout.destroy();
+
+			let { responseType = '' } = options;


agubler · 2016-11-21T14:10:28Z

src/request/fetch.ts

+	const requestUrl = generateRequestUrl(url, options);
+
+	if ((!options.user || !options.password) && options.auth) {
+		let auth = options.auth.split(':');


agubler · 2016-11-21T14:12:16Z

src/request/fetch.ts

+		let hasContentTypeHeader = false;
+		let hasRequestedWithHeader = false;
+
+		for (let header in headers) {


sorry about all the const suggestions...

agubler · 2016-11-21T14:13:28Z

src/request/fetch.ts

+				responseHeaders[ key.toLowerCase() ] = fetchResponse.headers.get(key);
+			});
+
+			Promise.resolve(body).then((body: any) => {


call this data and you can just use data in the resolved object and drop the : body

agubler · 2016-11-21T14:14:56Z

src/request/fetch.ts

+		if (options.timeout > 0 && options.timeout !== Infinity) {
+			timeout = (function (): Handle {
+				const timer = setTimeout(function (): void {
+					const error = new RequestTimeoutError('Request timed out after ' + options.timeout + 'ms');


how about using template literals

new RequestTimeoutError(`Request timed out after ${options.timeout}ms`);

agubler · 2016-11-21T14:23:09Z

src/request/fetch.ts

+					reject(error);
+				}, options.timeout);
+
+				return createHandle(function (): void {


can do if you so desire,

return createHandle((): void => { clearTimeout(timer); });

or perhaps even

timeout = ((): Handle => { const timer = setTimeout((): void => { const error = new RequestTimeoutError('Request timed out after ' + options.timeout + 'ms'); reject(error); }, options.timeout); return createHandle(clearTimeout.bind(null, timer)); })();

agubler · 2016-11-21T14:48:20Z

src/request/fetch.ts

+
+			const responseHeaders: ResponseHeaders = {};
+
+			forOf(fetchResponse.headers.keys(), (key: string) => {


another mainly styling suggestion (I think that works 😉 )

forOf(fetchResponse.headers.entries(), ([key, value]) => { responseHeaders[ key.toLowerCase() ] = value; });

agubler · 2016-11-21T14:55:00Z

src/request/fetch.ts

+
+			const responseHeaders: ResponseHeaders = {};
+
+			forOf(fetchResponse.headers.enrties(), ([key, value]) => {


Ugh. how did that build? ....

I really do not know!

codecov-io · 2016-11-21T17:28:33Z

Current coverage is 94.52% (diff: 100%)

Merging #243 into master will decrease coverage by 3.32%

@@             master       #243   diff @@
==========================================
  Files            47         48     +1   
  Lines          2457       2519    +62   
  Methods          28         29     +1   
  Messages          0          0          
  Branches        464        477    +13   
==========================================
- Hits           2404       2381    -23   
  Misses           51         51          
- Partials          2         87    +85

Powered by Codecov. Last update d2bbb57...1d35e74

agubler · 2016-11-21T18:09:23Z

src/request/fetch.ts

+					break;
+
+				case 'xml':
+					body = fetchResponse.text().then((asText: string) => {


I believe .then should be on a new line as per the style guide

agubler

Approved from me! Someone else might want eyes on before the big green light.

agubler · 2016-12-05T08:49:49Z

Not sure if anyone else will get the opportunity to review this 🎱

dylans · 2016-12-05T13:06:44Z

A few stray thoughts that are probably worth addressing mostly later as we decide how useful the fetch API might be:

the fetch API doesn't seem to have a convenience method for json, so the user must explicitly parse json? Is that how our request implementation works also?
the fetch API seems to implement ReadableStream... I assume we're planning to ignore that for now given the size of the streams shim?
are we ignoring the type Document (I see we have arraybuffer and blob), or does that get parsed the same way as plain text?
the fetch API has a number of scenarios around cors, requestType, requestDestination, etc. Do we plan to/need to support these features now or in the future?

rorticus · 2016-12-05T13:58:43Z

the fetch API doesn't seem to have a convenience method for json, so the user must explicitly parse json? Is that how our request implementation works also?

We have a response filter that runs at a "higher level" that will turn the json type into an actual JSON object.

the fetch API seems to implement ReadableStream... I assume we're planning to ignore that for now given the size of the streams shim?

I'm OK with ignoring that :) If we do decide to implement it, we should at least figure out how we're going to pull in streams without the dependency in the node request.

are we ignoring the type Document (I see we have arraybuffer and blob), or does that get parsed the same way as plain text?

It'll get parsed as plain text.

the fetch API has a number of scenarios around cors, requestType, requestDestination, etc. Do we plan to/need to support these features now or in the future?

That is a great question :) I've ignored them for now because we didn't have anything similar in the xhr/node requests, but adding those options might be a compelling reason why someone would want to use the fetch API over the alternatives.

rorticus · 2016-12-13T15:36:54Z

Dylan's comments have been split into #255 and #254 to get this landed.

Adding fetch implementation of request api, issue dojo#139

2720670

agubler reviewed Nov 21, 2016

View reviewed changes

rorticus added 2 commits November 21, 2016 08:09

Retyping fetch response body

4249f2d

Fixing warning

76278ce

agubler suggested changes Nov 21, 2016

View reviewed changes

Code style changes

f479945

agubler reviewed Nov 21, 2016

View reviewed changes

Code style changes

d92d32b

agubler reviewed Nov 21, 2016

View reviewed changes

Fixing typo

7796276

dylans added this to the 2016.11 milestone Nov 21, 2016

Removing iterator usage because it isnt supported in FF 39

08e91ce

agubler reviewed Nov 21, 2016

View reviewed changes

Style fixes

1d35e74

agubler approved these changes Nov 23, 2016

View reviewed changes

dylans modified the milestones: 2016.12, 2016.11 Dec 5, 2016

rorticus merged commit e5b8f09 into dojo:master Dec 13, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding fetch implementation of request api, issue #139 #243

Adding fetch implementation of request api, issue #139 #243

rorticus commented Nov 21, 2016

agubler Nov 21, 2016 •

edited

Loading

rorticus Nov 21, 2016

agubler left a comment

agubler Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler Nov 21, 2016

codecov-io commented Nov 21, 2016 •

edited

Loading

agubler Nov 21, 2016

rorticus Nov 21, 2016

agubler left a comment

agubler commented Dec 5, 2016

dylans commented Dec 5, 2016

rorticus commented Dec 5, 2016

rorticus commented Dec 13, 2016


		const responseHeaders: ResponseHeaders = {};

		forOf(fetchResponse.headers.keys(), (key: string) => {


		const responseHeaders: ResponseHeaders = {};

		forOf(fetchResponse.headers.enrties(), ([key, value]) => {

Adding fetch implementation of request api, issue #139 #243

Adding fetch implementation of request api, issue #139 #243

Conversation

rorticus commented Nov 21, 2016

agubler Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler left a 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

Choose a reason for hiding this comment

codecov-io commented Nov 21, 2016 • edited Loading

Current coverage is 94.52% (diff: 100%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agubler left a comment

Choose a reason for hiding this comment

agubler commented Dec 5, 2016

dylans commented Dec 5, 2016

rorticus commented Dec 5, 2016

rorticus commented Dec 13, 2016

agubler Nov 21, 2016 •

edited

Loading

codecov-io commented Nov 21, 2016 •

edited

Loading