Introduce rawRequest() to replace rawResponse#1317
Conversation
force f=json request and remove response formatting through a default request. passing f params to request should warn and override
remove unused params parameter as params should already be within the options parameter
remove redundant assignment destructure requestFlags and remove double self platform call destructure request context wrap rawRequest in retry logic for parity remove error retry in rawResponse restruct build auth logic
…into rawrequest-replace-rawresponse
rawResponse
rawResponserawRequest() to replace rawResponse
There was a problem hiding this comment.
Pull request overview
This PR introduces a new rawRequest() API in @esri/arcgis-rest-request to replace the legacy rawResponse behavior, while also migrating legacy top-level request options (e.g. httpMethod, headers, credentials, signal, hideToken, suppressWarnings) into the newer fetchOptions and requestFlags shapes.
Changes:
- Added
rawRequest()and refactoredrequest()/internalRequest()to centralize option normalization and deprecation warnings. - Added
normalizeDeprecatedRequestOptions()+ tests, and updated deprecated-option warning behavior (including asuppressWarningsargument). - Updated multiple packages/tests to use
fetchOptions.methodand to route raw-response use cases throughrawRequest()(including Feature Servicef=pbf).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/arcgis-rest-request/test/utils/normalize-deprecated-request-options.test.ts | Adds unit tests for legacy option normalization into requestFlags/fetchOptions. |
| packages/arcgis-rest-request/test/utils/ArcGISTokenRequestError.test.ts | Updates expectations to assert fetchOptions.method rather than legacy httpMethod. |
| packages/arcgis-rest-request/test/utils/ArcGISRequestError.test.ts | Updates expectations to assert fetchOptions.method rather than legacy httpMethod. |
| packages/arcgis-rest-request/test/utils/ArcGISAuthError.test.ts | Updates tests to pass fetchOptions.method instead of httpMethod. |
| packages/arcgis-rest-request/test/request.test.ts | Updates request behavior tests; adds rawRequest() coverage; switches legacy options to new shapes. |
| packages/arcgis-rest-request/test/deprecated-request-options-warning.test.ts | Updates warning assertions and adds suppressWarnings argument coverage. |
| packages/arcgis-rest-request/src/utils/warn-deprecated-request-options.ts | Adds “unsupported” warnings for maxUrlLength/rawResponse and supports suppression. |
| packages/arcgis-rest-request/src/utils/normalize-deprecated-request-options.ts | New helper to map legacy top-level options into requestFlags/fetchOptions. |
| packages/arcgis-rest-request/src/utils/IRequestOptions.ts | Removes deprecated request override type and related internal type alias. |
| packages/arcgis-rest-request/src/utils/append-custom-params.ts | Removes legacy request from the non-param key list. |
| packages/arcgis-rest-request/src/request.ts | Core refactor: normalization, rawRequest(), revised fetch flow, and updated token handling. |
| packages/arcgis-rest-request/src/fetch-token.ts | Removes now-unnecessary rawResponse=false forcing; renames response type. |
| packages/arcgis-rest-request/src/AuthenticationManagerBase.ts | Removes rawResponse note/override; relies on new request behavior. |
| packages/arcgis-rest-request/src/ArcGISIdentityManager.ts | Removes rawResponse note/override; relies on new request behavior. |
| packages/arcgis-rest-portal/src/sharing/is-item-shared-with-group.ts | Updates docs removing the outdated rawResponse limitation note. |
| packages/arcgis-rest-portal/src/items/get.ts | Switches file/blob paths to rawRequest() and removes rawResponse toggling via request(). |
| packages/arcgis-rest-geocoding/src/geocode.ts | Routes rawResponse behavior through rawRequest(). |
| packages/arcgis-rest-geocoding/src/bulk.ts | Routes rawResponse behavior through rawRequest(). |
| packages/arcgis-rest-feature-service/test/query.test.ts | Adds test ensuring f=pbf returns a raw Response. |
| packages/arcgis-rest-feature-service/test/query.test.live.ts | Removes rawResponse: true from live PBF tests (now implied via behavior). |
| packages/arcgis-rest-feature-service/test/addToServiceDefinition.test.ts | Updates expectations from httpMethod to fetchOptions.method. |
| packages/arcgis-rest-feature-service/src/query.ts | Uses rawRequest() for PBF and updates docs about PBF usage. |
| packages/arcgis-rest-feature-service/src/createFeatureService.ts | Removes outdated rawResponse limitation note and override. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fetchOptions = { | ||
| ...(httpMethod !== undefined ? { method: httpMethod } : {}), | ||
| ...(credentials !== undefined ? { credentials } : {}), | ||
| ...(signal !== undefined ? { signal } : {}), | ||
| ...requestOptions.fetchOptions, | ||
| headers: { | ||
| ...(headers as any), | ||
| ...(requestOptions.fetchOptions?.headers as any) | ||
| } | ||
| }; |
There was a problem hiding this comment.
I created helper methods to cover this in a more comprehensive way since it is needed in multiple places. We can reevaluate once all legacy shape options and normalization functions are removed if we need to keep it around still. (there should be one or two direct calls to merge headers in request standalone)
patrickarlt
left a comment
There was a problem hiding this comment.
@dixonyant this LGTM but I would take the advice of the Copilot bot and handle the header object in other formats then just a plain object.
…-options.ts Co-authored-by: Patrick Arlt <patrick.arlt@gmail.com>
| const options = { | ||
| httpMethod: "GET", | ||
| authentication: this, | ||
| ...requestOptions, | ||
| rawResponse: false | ||
| ...requestOptions | ||
| } as IRequestOptions; |
There was a problem hiding this comment.
These and all the below instances are all part of this larger wip change that is going into the v5.0.0 branch. The focus of this PR is to handle options at the request/rawRequest level. there is a follow up story that covers all the package-level and repo-uses of LegacyRequestOptions. These will all be done by the time v5.0.0 -> main merges into main.
| } else if (queryOptions.params?.f === "pbf") { | ||
| return rawRequest( | ||
| `${cleanUrl(requestOptions.url)}/query`, | ||
| queryOptions | ||
| ) as Promise<any>; |
There was a problem hiding this comment.
@dixonyant this also seems off. The goal is that all methods should return JSON by default and this breaks the pattern by returning Promise<any> if f=pbf so if someone really wants f=pbf they will neet to use a new rawQueryFeatures() that will need to be added at v5
There was a problem hiding this comment.
Yes it is off. This in-between behavior shouldn't even see the light of day since the follow-up PR is going to be all the removing of rawRequest calls from all functions that should be only returning JSON. They either get a replacement rawFunction call after v5 or with v5. Just the purpose of this PR is to replace the guts of all calls to request({rawResponse}) with rawRequest for immediate backwards compatibility internally, and then I was going to remove fully and assert types in the next follow-up PR.
Minimum allowed line rate is |
patrickarlt
left a comment
There was a problem hiding this comment.
@dixonyant There still seem to be a few cases where methods can return non JSON objects. These could get resolved now or in a future issue if you want to make a follow up issue.
| } else if (queryOptions.params?.f === "pbf") { | ||
| return rawRequest( | ||
| `${cleanUrl(requestOptions.url)}/query`, | ||
| queryOptions | ||
| ) as Promise<any>; |
There was a problem hiding this comment.
@dixonyant this also seems off. The goal is that all methods should return JSON by default and this breaks the pattern by returning Promise<any> if f=pbf so if someone really wants f=pbf they will neet to use a new rawQueryFeatures() that will need to be added at v5
This PR introduces
rawRequest()to replacerawResponse: truein rest-request. It also implements the new IRequestOptions interface across request and request tests.It outright removes support for
rawResponseandmaxUrlLengthwhile retaining backwards-compatibility for legacy IRequestOptions.Users will have to update their code to use
requestFlags.ignoreMaxUrlLengthand manually request a raw response usingrawRequest()and handle the response on their own.Raw Request:
- to be used to request a response as html, text, pbf, or BLOB.
IRequestOptions:
- moved http related options to
requestOption.fetchOptions- moved boolean flags to
requestOptions.requestFlags- refactored
maxUrlLengthcase to a new workflow that usesrequestFlags.ignoreMaxUrlLengthwhen users want to force a GET request with a very long url length (due to long query params or other large stringified properties appended to url)- removed
requestfrom IRequestOptions as it was an artifact from jasmine era testing daysAdded
normalizeDeprecatedRequestOptions()as a way to support backwards compatibility for ILegacyRequestOptions (except forrawResponseandmaxUrlLengthwhich we are fully removing from request in this pr).