Update IRequestOptions interface to v5 spec#1311
Conversation
upgrade to native node and web api fetch in v5 should make this unnecessary
…questoptionsv5changes
patrickarlt
left a comment
patrickarlt
left a comment
There was a problem hiding this comment.
@dixonyant I have a few small things in the PR plus a request about refactoring the internalRequest() logic. Which could be in this PR or the future PR where you add rawRequest().
| "@esri/arcgis-rest-auth": major | ||
| --- | ||
|
|
||
| Changed IRequestOptions interface to have only necessary top level properties, moved fetch and request options to fetchOptions and requestFlags |
There was a problem hiding this comment.
@dixonyant Can you enumerate all the options that changed here and what they changed to?
There was a problem hiding this comment.
what will happen to "maxUrlLength"? @patrickarlt will it need to be removed or just moved somewhere else?
| }); | ||
| } | ||
| if (rawResponse) { | ||
| console.warn( |
There was a problem hiding this comment.
rawResponse is actually deprecated to I think we need to rethink the roles of internalRequest() and request().
Currently request() exists to handle the request option and all the logic happens in internalRequest().
I think with our new model this will be different. We know request() will always be getting JSON so all the error and response processing, error retry, error checking etc... should probably happen in request().
Then inside internalRequest() we can do all the processing that needs to happen before the request like default options, etc... basically anything that we don't need to know about the response for.
Another thing that might help would be to break out as much of this logic as possible into utility functions that we can test in smaller increments.
There was a problem hiding this comment.
I 100% agree. Right now request is just calling internalRequest with a retry loop. Agreed that a lot of it can be broken down into helpers and updated to async/await syntax for legibility. I was going to handle gutting the rawResponse with adding rawRequest in the same future PR.
There was a problem hiding this comment.
Sounds good we can merge this is an you can follow up with:
- Add
rawRequest() - Refactor
internalRequest()andrequest()logic to supportrawRequest()
Co-authored-by: Patrick Arlt <patrick.arlt@gmail.com>
Minimum allowed line rate is |
2 participants
Moving all request options into a legacy options interface that IRequestOptions will extend. IRequestOptions will only expose
params,authentication,portal,requestOptions, andfetchOptions. All legacy options will be deprecated and some will be removed altogether in coming pr's.Closes: #1292, #880