Use the Fetch API instead of SuperAgent to communicate with MS Graph #63

ghost · 2018-02-02T01:52:12Z

MS Graph JS SDK should break the dependency on SuperAgent

MS Graph SDK is currently using SuperAgent behind the scenes to communicate with the Microsoft Graph. This dependency should be reconsidered for the following reasons:

SuperAgent is using XHR behind the scenes.

While there isn’t anything inherently wrong with XHR, usage of this technology has been “deprecated” for the Fetch API (https://blogs.windows.com/msedgedev/2016/05/24/fetch-and-xhr-limitations/ for more information). Switching to the Fetch API would align MSGraph SDK with the industry standard method for making network requests.

SuperAgent + it’s dependencies is large.

A quick look at graph-js-sdk-web.js reveals that SuperAgent is a large majority of the file’s contents and is only being used to make XHR calls. The Fetch API polyfill is much smaller and will likely simplify the code used to make network requests to Microsoft Graph in the SDK.

Additional "Bug"
Setting the responseType to Document will result in JSON being returned.
Fetch API doesn't support this feature (whatwg/fetch#16) This return type doesn't really make sense for communicating with Microsoft Graph.

Requests are now processed using the fetch API Response Handler needs some refactoring to handle the error vs bad response

ghost · 2018-02-02T18:35:58Z

package.json

    "@types/mocha": "^2.2.34",
+    "@types/node": "^9.4.0",


Cannot successfully build this library without node typings for require statements

MIchaelMainer · 2018-02-06T06:59:35Z

First off, thank you @lahuey for taking the time to propose this change. This makes sense to me. I apologize as I can't get to review this right now. We will definitely take a look at this when we get back to this repo.

Solid.

MIchaelMainer · 2018-04-13T18:17:48Z

package.json

    "browserify": "^13.1.0",
    "mocha": "^3.2.0",
-    "typescript": "^2.2.1"
+    "typescript": "^2.2.1",
+    "whatwg-fetch": "^2.0.3"


Global: we should be using an isomorphic Fetch library so that we are covered in the browser and from Node. Consider the following:

https://github.com/matthew-andrews/isomorphic-fetch

https://github.com/lquixada/cross-fetch

MIchaelMainer · 2018-04-13T20:24:13Z

spec/core/responseHandling.ts

-    a: 1
-  }
-};
+const _200_SUPERAGENT_RES_BODY: any = { a: 1 }; 


Global: Remove references to superagent. Change to Fetch, or leave it out all together.

MIchaelMainer · 2018-04-13T22:26:37Z

spec/core/responseHandling.ts


+const error: Error = new Error();
+(error as any).statusCode = 404;



Were you able to run the tests? I wasn't able to from your fork. I then updated to use cross-fetch - two of the tests aren't passing.

27 passing (19ms) 2 failing 1) #ParseResponse() extracts code and request-id from the JSON body of 500 errors: TypeError: Cannot read property 'code' of null at lib\spec\core\responseHandling.js:38:29 at Function.ResponseHandler.init (lib\src\ResponseHandler.js:8:13) at Context.<anonymous> (lib\spec\core\responseHandling.js:37:43) 2) #ParseError() parses a 404 superagent error: AssertionError: -1 == 404 + expected - actual --1 +404 at lib\spec\core\responseHandling.js:52:20 at Function.ResponseHandler.init (lib\src\ResponseHandler.js:17:17) at Context.<anonymous> (lib\spec\core\responseHandling.js:51:43)

Switch to isomorphic fetch

mpasarin · 2018-04-16T22:01:15Z

Agreed on the comments for isomorphic fetch, and ensuring that the tests pass.
After that, approved from my side.

MIchaelMainer · 2018-04-17T00:23:45Z

I updated the base branch to pull into. There are some issues that are popping up when I run npm run build that need to be resolved. An array declaration like this: [string], instead of string[]. You can see it in src/GraphRequest.ts (107,32) and in spec/core/urlGeneration.ts(24,25).

Please resolve the merge conflicts and we should be good.

MIchaelMainer · 2018-04-17T22:34:54Z

src/GraphRequest.ts

@@ -199,38 +199,40 @@ export class GraphRequest {

    delete(callback?:GraphRequestCallback):Promise<any> {
        let url = this.buildFullUrl();
-        return this.sendRequestAndRouteResponse(request.del(url), callback)
+        return this.sendRequestAndRouteResponse(
+            new Request(url, { method: RequestMethod.DELETE, headers: new Headers() }),


We aren't passing in the headers set by the developer. We are always setting the headers collection to an empty collection. We should set the headers passed in by the user and only set an empty header collection if it is a required argument.

For DELETE, I think we should not have a default header.

The headers are set on this._headers. We should check that first for headers.

There are always headers present
headers.append('Authorization', 'Bearer ' + accessToken);
headers.append('SdkVersion', "graph-js-" + packageInfo.version);

MIchaelMainer · 2018-04-17T22:36:51Z

src/GraphRequest.ts

-            request
-                .patch(url)
-                .send(content),
+            new Request(url, { method: RequestMethod.PATCH, body: content, headers: new Headers() }),


We aren't passing in the headers set by the developer. We are always setting the headers collection to an empty collection. We should set the headers passed in by the user and only set an empty header collection if it is a required argument.

For PATCH, I think we should have a default content-type: application\json header set if no headers are provided by the customer.

MIchaelMainer · 2018-04-17T22:41:08Z

src/GraphRequest.ts

-                .post(url)
-                .send(content),
-                callback
+            new Request(url, { method: RequestMethod.POST, body: content, headers: new Headers() }),


We aren't passing in the headers set by the developer. We are always setting the headers collection to an empty collection. We should set the headers passed in by the user and only set an empty header collection if it is a required argument.

For POST, I think we should have a default content-type: application\json header set if no headers are provided by the customer.

Sound good to me

MIchaelMainer · 2018-04-17T22:47:44Z

src/GraphRequest.ts

+                { 
+                    method: RequestMethod.PUT,
+                    body: content,
+                    headers: new Headers({ 'Content-Type' : 'application/octet-stream' })


Same notes as for the other verbs. We shouldn't set the default content-type to application/octet-stream. We have many other PUT scenarios that use different content types.

https://developer.microsoft.com/en-us/graph/docs/api-reference/v1.0/api/profilephoto_update

@MIchaelMainer wouldn't this be a breaking change? Isn't the original code setting this header?

Shoot. Yes, you're right. This would be breaking. Unfortunately, this implementation is wrong. We can Put other content types in other calls plus, there is a PutStream function that also hard codes this content type. For now, we should leave this broke (not happy about this).

Example
https://developer.microsoft.com/en-us/graph/docs/api-reference/beta/api/team_put_teams

@lahuey Can you make it so that when the customer sets the content-type header, it checks whether it exists and it removes the existing one if it does. That way, we won't break existing customers who have taken a dependency on this behavior.

changed this.configureRequest to use Headers.set instead of Headers.append

User headers now overwrite default headers

bullsseye · 2018-05-31T10:14:07Z

@lahuey can you please pull below commit into your branch. It fixes the non get calls.

commit: 574974a

I tried to push it into your branch, but it is giving me permission denied. This commit is reviewed by @MIchaelMainer

Changed [string] to string[], fixed spacing, fixed double parens

ghost · 2018-05-31T22:28:06Z

@bullsseye I added you as a collaborator so you should be able to push now. Sorry about that!

bullsseye · 2018-06-05T08:13:21Z

@lahuey I am using the below command to push the commit: 574974a but it is still giving me permission denied.

command: git push https://github.com/lahuey/msgraph-sdk-javascript fix-POST-PATCH-PUT-request:RemoveSuperAgentDependency

error that I receive: To https://github.com/lahuey/msgraph-sdk-javascript
! [remote rejected] fix-POST-PATCH-PUT-request -> RemoveSuperAgentDependency (permission denied)
error: failed to push some refs to 'https://github.com/lahuey/msgraph-sdk-javascript'

where fix-POST-PATCH-PUT-request is my local created out of your pull request on which this commit: 574974a is created.

Please let me know if I need to do something different to push this.

Other way is that you pull this branch: https://github.com/microsoftgraph/msgraph-sdk-javascript/commits/fix-POST-PATCH-PUT-request into your local and then push that into this pull request. This branch is Master + Your pull request + fix commit

…om U-FAREAST\ramgupt

ghost · 2018-06-05T17:29:59Z

@bullsseye just merged your changes into my branch

bullsseye · 2018-06-12T11:30:15Z

@MIchaelMainer Please review this commit: 584c60f

It includes the test cases that are added to verify this pull request

bullsseye · 2018-06-13T04:12:34Z

@MIchaelMainer Comments are fixed in the commit: 60270db

MIchaelMainer · 2018-06-13T16:32:14Z

@bullsseye Reviewed 60270db, added a comment about where to add the access token. I also added statements for the test assumptions. I think that stating those assumptions are optional, but beneficial.

bullsseye · 2018-06-14T02:28:31Z

@MIchaelMainer I have fixed the comments in this commit 946253f

MIchaelMainer · 2018-06-14T06:27:37Z

👍 Please pull it

bullsseye · 2018-06-15T03:52:41Z

@lahuey Please pull this commit: 946253f

Post this we will merge this branch

ghost · 2018-06-18T16:56:31Z

@bullsseye Awesome. Just pushed the commit manually again (Need to figure out how to use GitHub at some point)

bullsseye · 2018-06-20T03:46:50Z

src/GraphRequest.ts

-                                this.urlComponents.version,
-                                this.urlComponents.path])
-                  + this.createQueryString();
+        this.urlComponents.version,


previous indentation was correct. Not a blocker though

bullsseye · 2018-06-20T03:51:20Z

src/GraphRequest.ts

        );
    }


    // Given the built SuperAgentRequest, get an auth token from the authProvider, make the request and return a promise
-    private routeResponseToPromise(requestBuilder:request.SuperAgentRequest) {
+    private routeResponseToPromise(request: Request) {


Update the above comment : not a blocker

bullsseye · 2018-06-20T04:07:01Z

src/GraphRequest.ts

@@ -271,12 +284,17 @@ export class GraphRequest {
    }

    // Given the built SuperAgentRequest, get an auth token from the authProvider, make the request and invoke the callback
-    private routeResponseToCallback(requestBuilder:request.SuperAgentRequest, callback: GraphRequestCallback) {


Update the above comment: not a blocker

bullsseye · 2018-06-20T04:10:57Z

Merging this, There are non blocker changes required on this.

MarkHerhold · 2018-06-20T15:30:51Z

This is really great! Any idea when we can expect a release with these changes?

REDMOND\lahuey added 2 commits December 13, 2017 16:32

Removed dependency on superagent

d2636ca

Requests are now processed using the fetch API Response Handler needs some refactoring to handle the error vs bad response

Fixed bugs several small bugs

f31d3e9

ghost changed the base branch from dev to master February 2, 2018 17:52

ghost commented Feb 2, 2018

View reviewed changes

MIchaelMainer reviewed Apr 13, 2018

View reviewed changes

Update unit test

c00fac2

Switch to isomorphic fetch

for each

eca7fd6

MIchaelMainer changed the base branch from master to dev April 17, 2018 00:14

MIchaelMainer changed the base branch from dev to fetchApiUpdate April 17, 2018 00:16

update javascript file

b5d0a22

MIchaelMainer reviewed Apr 17, 2018

View reviewed changes

REDMOND\lahuey and others added 4 commits April 26, 2018 11:26

added default content type to POST and PATCH

e5d9d97

User headers now overwrite default headers

Merge branch 'fetchApiUpdate' into lahuey/RemoveSuperAgentDependency

ffc0cf5

Resolved merge conflicts

2d22130

Changed [string] to string[], fixed spacing, fixed double parens

cc0f188

Merge pull request #2 from lahuey/buildErrorFix

8b4fce3

Changed [string] to string[], fixed spacing, fixed double parens

Manually pulled "Pass the body of POST/PUT/PATCH calls correctly." fr…

89ab20b

…om U-FAREAST\ramgupt

MIchaelMainer assigned bullsseye Jun 6, 2018

bullsseye assigned muthurathinam and unassigned bullsseye Jun 11, 2018

muthurathinam assigned bullsseye and unassigned muthurathinam Jun 12, 2018

Manually pulled "Add tests" from U-FAREAST\ramgupt

43c6f84

bullsseye reviewed Jun 20, 2018

View reviewed changes

bullsseye merged commit 55c308a into microsoftgraph:fetchApiUpdate Jun 20, 2018

muthurathinam mentioned this pull request Aug 13, 2018

Update README.md with download as binary example #96

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the Fetch API instead of SuperAgent to communicate with MS Graph #63

Use the Fetch API instead of SuperAgent to communicate with MS Graph #63

ghost commented Feb 2, 2018

ghost Feb 2, 2018

MIchaelMainer commented Feb 6, 2018 •

edited

Loading

MIchaelMainer Apr 13, 2018

MIchaelMainer Apr 13, 2018

MIchaelMainer Apr 13, 2018

mpasarin commented Apr 16, 2018

MIchaelMainer commented Apr 17, 2018 •

edited

Loading

MIchaelMainer Apr 17, 2018

ghost Apr 26, 2018 •

edited by ghost

Loading

MIchaelMainer Apr 17, 2018

MIchaelMainer Apr 17, 2018

ghost Apr 26, 2018

MIchaelMainer Apr 17, 2018

ghost Apr 26, 2018

MIchaelMainer Apr 26, 2018

MIchaelMainer Apr 26, 2018

ghost Apr 26, 2018

bullsseye commented May 31, 2018 •

edited

Loading

ghost commented May 31, 2018

bullsseye commented Jun 5, 2018 •

edited

Loading

ghost commented Jun 5, 2018

bullsseye commented Jun 12, 2018 •

edited

Loading

bullsseye commented Jun 13, 2018

MIchaelMainer commented Jun 13, 2018

bullsseye commented Jun 14, 2018 •

edited

Loading

MIchaelMainer commented Jun 14, 2018

bullsseye commented Jun 15, 2018

ghost commented Jun 18, 2018

bullsseye Jun 20, 2018

bullsseye Jun 20, 2018

bullsseye Jun 20, 2018

bullsseye commented Jun 20, 2018

MarkHerhold commented Jun 20, 2018


		const error: Error = new Error();
		(error as any).statusCode = 404;

Use the Fetch API instead of SuperAgent to communicate with MS Graph #63

Use the Fetch API instead of SuperAgent to communicate with MS Graph #63

Conversation

ghost commented Feb 2, 2018

MS Graph JS SDK should break the dependency on SuperAgent

SuperAgent is using XHR behind the scenes.

SuperAgent + it’s dependencies is large.

Choose a reason for hiding this comment

MIchaelMainer commented Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpasarin commented Apr 16, 2018

MIchaelMainer commented Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

ghost Apr 26, 2018 • edited by ghost 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

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

bullsseye commented May 31, 2018 • edited Loading

ghost commented May 31, 2018

bullsseye commented Jun 5, 2018 • edited Loading

ghost commented Jun 5, 2018

bullsseye commented Jun 12, 2018 • edited Loading

bullsseye commented Jun 13, 2018

MIchaelMainer commented Jun 13, 2018

bullsseye commented Jun 14, 2018 • edited Loading

MIchaelMainer commented Jun 14, 2018

bullsseye commented Jun 15, 2018

ghost commented Jun 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bullsseye commented Jun 20, 2018

MarkHerhold commented Jun 20, 2018

MIchaelMainer commented Feb 6, 2018 •

edited

Loading

MIchaelMainer commented Apr 17, 2018 •

edited

Loading

ghost Apr 26, 2018 •

edited by ghost

Loading

bullsseye commented May 31, 2018 •

edited

Loading

bullsseye commented Jun 5, 2018 •

edited

Loading

bullsseye commented Jun 12, 2018 •

edited

Loading

bullsseye commented Jun 14, 2018 •

edited

Loading