-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
WIP: Skip XHR processing #9448
WIP: Skip XHR processing #9448
Conversation
* @return {!Promise<!JSONType>} | ||
*/ | ||
fetchJson(input, opt_init) { | ||
fetchJson(input, opt_init, opt_skipProcessing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a too big change to the API? Basically the @return
is no longer honored. Or am I missing something? I think it'd be better to provide separate APIs if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just return the response always? It's not that big of a deal to call #json
or #text
in the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't understand what this does. Is fetchJson(input, init, true)
equivalent to fetch(input, init, true)
in this world? If so, then surely we could just have the second one; this API change is only useful to clients who are asking for the raw response.
Or is the idea that fetchJson(input, init, true)
should do the fetch and then transform the response body to JSON without regard to the status code? I think that would also be okay (although I'm uncertain whether anyone actually needs it, and having clients doing something different just use fetch
seems fine). But I don't think that's what this does now.
Or am I missing something and the intended functionality is something else entirely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#fetchJSON
and #fetch
won't be equivalent, since they do different processing before making the request (normalizing Content-Type
, Accept
, etc). This just make it so they both return Response
regardless of anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that always returning the response is better than flipping between them. How often do we process errors anyway though? The current signature is basically: "don't care about exact errors - just get me the JSON if you can". How often is this really not the case and shouldn't we just be able to get that data from the catch
branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this only has to do with JSON, let's maybe create a separate fetchJsonResponse
method and use it in fetchJson
and then we'll see if we can kill the older method if it gives very little value on top of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dvoytenko, in future I expect to use the fetch
API as augmented in this PR for non-JSON data. Or do you just mean fetchJson
in particular and the changes to fetch
are fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, fetch
itself should always return non-asserted response. The closer we are to fetch API for fetch
method specifically - the better. fetchJson
is a different story a bit, since there's no point to parse JSON when there's no content there. But overall, I also see value getting closer to the fetch
API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How often do we process errors anyway though?
In 99% of cases, the caller only wants successful JSON. But there's need in A4A for the response headers and JSON (or maybe it's text), and now Forms needs the successful or failure JSON. AMP Access could use it too.
But all of the normal callers don't care about error JSON, so if the JSON decode at the of xhr.fetchJSON().then(resp => resp.json())
fails because of bad parsing or because of network issues, it's the same "Oh well, request failed" to them. So they keep the same API regardless (just add a then(res => res.json())
) and their success and failure cases are the same. But for the ones that actually need the Response and it's body, they can take advantage of the more generic API.
In principle, this could be extracted from the catch block, and my current WIP code does that, but this complicates the code and undermines type safety, so it's not ideal.
We essentially have 3 cases: Success, Server error, Network Failure. But server error can return Response data (headers, or body), and so it's essentially a success. Having server errors go through the network failure path (#catch
blocks) is a bad API.
let's maybe create a separate fetchJsonResponse method and use it in
I'm gonna reject this one, because I've already ripped out a #fetchJsonResponse
method once. What #fetchJson
should really do is "setup a JSON XHR message, make the request, and give me the response".
fetchJson is a different story a bit, since there's no point to parse JSON when there's no content there
This is covered by the above 3 paths I mentioned. To the average caller, failure due to no JSON body (because it's a 404 or whatever) is the same error condition as network failure. So, adding the then(res => res.json())
will get them to the same spot:
// old API
xhr.fetchJson(url).then(json => {
// Do stuff
}, () => {
// Log some user message
});
// New API
xhr.fetchJson(url).then(res => {
// If network failed, never called. Go straight to #catch block below
// If response received and theres no body due to 404, then this will go to #catch below
// Else, go to #then below
return res.json()
}).then(json => {
// Do stuff
}, () => {
// Log some user message
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Let's then go ahead and switch API to always return the Response
. I'm happy with this - it gets us very close to the actual fetch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I like the approach. This should more cleanly address the "decorated error" weirdness than the way I tried with FetchError.
Partial for ampproject#9448.
* XHR#fetchText now returns Response Partial for #9448. * Lint * Fix test error
Allow XHR fetches to skip 2XX assertion and any additional processing (basically, just return me the damn response).
This gives the caller access to needed data (response headers, status, body) if they explicitly need it. For the average caller, nothing changes.
This also removes
FetchError
. There's no telling how that "error" instance will be passed around, and anything that doesn't have a#message
and#stack
is a danger to our error reporting process.Fixes #8017.
Fixes #9501.
Closes #7140.
/cc @taymonbeal