redux-offline
redux-offline copied to clipboard
sorodrigo
4xx errors don't discard requests
I've found that 4xx errors are not discarding requests with the default configs, going against what the docs says. I think this was working correctly on the @jevakallio code...
~Am able to confirm.~ Unfortunately only through an actual application and not with integration tests that should capture the whole process. Time to do some digging...
It is currently broken in the original repo as well. So at least I didn't break anything.
Oops, forgot I had overridden the default effect reconciler. The last published version of @redux-offline/redux-offline is working for me after all. Guess I'm going to need more information. Which version are you on?
Could you provide some code reproducing the issue? I added an example app if you need a starting point.
Sorry, didn't see your previous message. I have just got a 404 and it got discarded inmediatly, don't know if it's related to that specific error code... I'll try to find time to replicate it.
I think I've found the source of the problem.
Our problem happened because our server didn't send CORS headers when returning a 4xx error, so on default config.effect() fetch() was giving a network error due to lack of CORS headers, showing them on the console by the web browser. Point is, this network error is not being captured, going the Promise rejection up to send() function. There discard checks are only done over HTTP statuses, and by checking status in error, also when it's undefined or null it returns true so it check the status being between 400 and 500, that for both undefined and null is false, so request is not discarded and instead it's retried.
I don't know if this is the intended behaviour because not adding CORS headers is a server error and so it should be retried, but on the other hand, check for non network errors is bad and should discard anything that's low level and don't have defined a status, so I don't know in case of CORS errors should not do rollback because the request has already been processed by the server, if use the cors mode always with the fetch() function, or add an option to set if on demand, at least on the docs...
I have added some tests and they confirm the point with the error.status field being null. I'll add some changes to fix it and update the pull-request.
I have added a fix and now tests for discard status are passing. I think the problem with the retries on 4xx will not happen again.
This was intentional. If fetch fails to communicate with the server at all, maybe the user is offline, fetch will reject with a TypeError, which does not have a status. This is why discard() uses 'status' in error, it is testing for these sorts of errors.
Granted, a TypeError can also represent something wrong with your code, either on the client or server, where retrying won't really change anything. But fetch makes it difficult (/impossible?) to tell these errors apart. So I'm not sure what we can do to help users find such errors.
Also, I'm not sure how error.status can ever be null. If the promise from fetch resolves, it resolves with a valid response, which necessarily has a valid status.
This was intentional. If fetch fails to communicate with the server at all, maybe the user is offline, fetch will reject with a TypeError, which does not have a status. This is why discard() uses 'status' in error, it is testing for these sorts of errors.
Shouldn't network status be checked in advance? Although it would be check here too as a failsafe...
Granted, a TypeError can also represent something wrong with your code, either on the client or server, where retrying won't really change anything. But fetch makes it difficult (/impossible?) to tell these errors apart. So I'm not sure what we can do to help users find such errors.
I don't think a server error would throw a TypeError. Regarding checking appart this kind of errors, maybe we could check the message error if they say something interesting...
Also, I'm not sure how error.status can ever be null. If the promise from fetch resolves, it resolves with a valid response, which necessarily has a valid status.
Probably never, maybe it only would have ever a number or undefined, I did it just to have some failback. Maybe a better check would be Number.isSafeInteger(error.status)?
Shouldn't network status be checked in advance?
It is checked, but the check is not reliable. Specifically it is subject to false positives: Navigator.onLine.
Regarding checking appart this kind of errors, maybe we could check the message error if they say something interesting...
Maybe. The specification does not say what messages should be used for the different errors, only that they are TypeError. So browsers are free to use whatever messages they want. Or none at all. So you would have to investigate the responses in all the major browsers to see what is used in practice.
new Response(null, { status: null })
// Uncaught RangeError: Failed to construct 'Response': The status provided (0) is outside the range [200, 599].
new Response(null, { status: [] })
// Uncaught RangeError: Failed to construct 'Response': The status provided (0) is outside the range [200, 599].
const response = new Response(null, { status: undefined})
// response.status => 200
Response doesn't have a setter for status, so the constructor is the only way to set it. If status is undefined, it defaults to 200. Otherwise the provided value is converted to an unsigned int.
So you would have to investigate the responses in all the major browsers to see what is used in practice.
Is there any way where could I check the possible error fetch() could dispatch? Maybe the spec?
Response doesn't have a setter for status, so the constructor is the only way to set it. If status is undefined, it defaults to 200. Otherwise the provided value is converted to an unsigned int.
So if status is undefined, we can be sure it's not a response :-)
You can check the spec, but almost all errors fetch() throws are TypeError, and the spec doesn't specify what message should be used.
To summarize, most failures result in the response being set as a 'network error', which is mostly just a response with a status of 0, which the fetch() method responds to by throwing a TypeError.
The only other error I remember is RangeError, which was mostly(/entirely?) used for invalid status codes.