search

dojo / core

:rocket: Dojo 2 - language helpers and utilities.
http://dojo.io
Other
213 stars 62 forks source link

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

Closed rorticus closed 7 years ago

rorticus commented 7 years ago

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

codecov-io commented 7 years ago

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 commented 7 years ago

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

dylans commented 7 years ago

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

rorticus commented 7 years ago

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 commented 7 years ago

Dylan's comments have been split into https://github.com/dojo/core/issues/255 and https://github.com/dojo/core/issues/254 to get this landed.