Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(request): adds option to return the raw fetch response #463

Merged
merged 2 commits into from
Feb 16, 2019

Conversation

tomwayson
Copy link
Member

@tomwayson tomwayson commented Feb 15, 2019

You can now pass the rawResponse:true option to indicate that a function should return the fetch response directly to the client. This is useful for piping the response to a stream.

AFFECTS PACKAGES:
@esri/arcgis-rest-auth
@esri/arcgis-rest-feature-service-admin
@esri/arcgis-rest-feature-service
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-request
@esri/arcgis-rest-sharing
stream-response-to-file

ISSUES CLOSED: #462

request() now accepts a new rawResponse boolean option to indicate that it should return the fetch
response directly to the client.
This is useful for piping the response to a stream.

AFFECTS PACKAGES:
@esri/arcgis-rest-auth
@esri/arcgis-rest-feature-service-admin
@esri/arcgis-rest-feature-service
@esri/arcgis-rest-geocoder
@esri/arcgis-rest-sharing
stream-response-to-file

ISSUES CLOSED: #462
@tomwayson tomwayson requested a review from jgravois February 15, 2019 19:41
@tomwayson
Copy link
Member Author

@jgravois FYI - for some fns like the geocoding and getFeature() I had them return the raw response if that option was passed. For the rest I forced rawResponse: false b/c it wouldn't make sense. I doc'd that it was not suppoerted for all fns except any having to do w/ fetching tokens.

I also updated the demo by:

  • using queryFeatures() instead of request()
  • adding the rest-js packages as deps and fixed require()
  • adding a README

@codecov
Copy link

codecov bot commented Feb 15, 2019

Codecov Report

Merging #463 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #463   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          94     94           
  Lines        1202   1209    +7     
  Branches      216    219    +3     
=====================================
+ Hits         1202   1209    +7
Impacted Files Coverage Δ
packages/arcgis-rest-auth/src/UserSession.ts 100% <ø> (ø) ⬆️
packages/arcgis-rest-geocoder/src/bulk.ts 100% <100%> (ø) ⬆️
packages/arcgis-rest-geocoder/src/geocode.ts 100% <100%> (ø) ⬆️
packages/arcgis-rest-sharing/src/group-sharing.ts 100% <100%> (ø) ⬆️
...es/arcgis-rest-feature-service-admin/src/create.ts 100% <100%> (ø) ⬆️
packages/arcgis-rest-auth/src/fetch-token.ts 100% <100%> (ø) ⬆️
packages/arcgis-rest-feature-service/src/query.ts 100% <100%> (ø) ⬆️

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 b2e5a4d...e050481. Read the comment docs.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like your style dude.

dude

@@ -662,6 +662,7 @@ export class UserSession implements IAuthenticationManager {
* })
* ```
*
* @param requestOptions - Options for the request. NOTE: `rawResponse` is not supported by this operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can't help but feel like we're ignoring the fact that this is TypeScript by just adding a NOTE: to the documentation here.

i also can't think of an interface name that isn't insane, but we could create one:

export interface IRawResponseNotAllowedRequestOptions  {
  // everything that *was* in IRequestOptions
}

export interface IRequestOptions extends IProcessedRequestOptions {
  rawResponse?: boolean;
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I say we merge this as is and kick this down the line, until at least the docs can show inherited properties.

But if you're interested, I pushed my WIP on this to #465

@@ -160,7 +160,7 @@ export interface ICreateServiceResult {
* });
* ```
* Create a new [hosted feature service](https://developers.arcgis.com/rest/users-groups-and-items/create-service.htm). After the service has been created, call [`addToServiceDefinition()`](../addToServiceDefinition/) if you'd like to update it's schema.
* @param requestOptions - Options for the request
* @param requestOptions - Options for the request. NOTE: `rawResponse` is not supported by this operation.
* @returns A Promise that resolves with service details once the service has been created
*/
export function createFeatureService(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see pedantry ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that ICreateServiceRequestOptions doesn't inherit directly from IRequestOptions, and the intermediary interfaces are used by fns that do support rawResponse.

@@ -163,7 +163,7 @@ function changeGroupSharing(
/**
* Find out whether or not an item is already shared with a group.
*
* @param requestOptions - Options for the request.
* @param requestOptions - Options for the request. NOTE: `rawResponse` is not supported by this operation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see pedantry ^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

So in both these cases, we'd have to either duplicate all the otherwise inherited props, or disallow rawResponse in the entire tree of inherited interfaes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which is probably fine, I just don't want to work that hard.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that ICreateServiceRequestOptions doesn't inherit directly from IRequestOptions, and the intermediary interfaces are used by fns that do support rawResponse.

we'd have to either duplicate all the otherwise inherited props, or disallow rawResponse in the entire tree of inherited interfaces.

💡

i don't blame you.

@tomwayson tomwayson merged commit cbbaed9 into master Feb 16, 2019
@jgravois jgravois deleted the fix/wrapped-raw-response branch February 16, 2019 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants