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

Make FetchRequest not try and parse a 204 response body #1773

Merged
merged 1 commit into from
May 21, 2024

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented May 21, 2024

It currently checks for the presence of a Content-Type response header and if present assumes that trying to parse the response will succeed.

Realtime currently sends a 204 No Content with a Content-Type header from the POST rest.ably.io/push/publish endpoint when using HTTP 1.1 (see REA-1924).

(This bug hasn't been caught by the tests because I guess most of the time they use HTTP 2 in browsers, and also we don't use FetchRequest in the tests except briefly in those for the modular variant of the library. I’ve created #1772 for improving this situation).

The 204 handling approach is copied from that which we already have in XHRRequest.

Resolves #1771.

It currently checks for the presence of a Content-Type response header
and if present assumes that trying to parse the response will succeed.

Realtime currently sends a 204 (No Content) with a Content-Type header
from the POST rest.ably.io/push/publish endpoint when using HTTP 1.1
(see REA-1924).

(This bug hasn't been caught by the tests because I guess most of the
time they use HTTP 2 in browsers, and also we don't use FetchRequest in
the tests except briefly in those for the modular variant of the
library. I’ve created #1772 for improving this situation).

The 204 handling approach is copied from that which we already have in
XHRRequest.

Resolves #1771.
Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

LGTM

@lawrence-forooghian lawrence-forooghian merged commit 04f3c89 into main May 21, 2024
12 checks passed
@lawrence-forooghian lawrence-forooghian deleted the 1771-FetchRequest-handle-204 branch May 21, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

FetchRequest doesn't properly handle a 204 response
2 participants