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

add streaming code #372

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

add streaming code #372

wants to merge 18 commits into from

Conversation

allanchau
Copy link
Member

@allanchau allanchau commented Mar 8, 2024

This PR adds support for stream responses.

Verification and testing

Testing

  • Check the tests are passing.
  • Connect this pr to a project and check it works as it did previously.
  • Check it works if a response is streamed eg stream.pipe(res).

Deployment

  • Release the patch version.
  • Updated related prs with the new version.

@allanchau allanchau marked this pull request as ready for review March 11, 2024 00:26
@allanchau allanchau requested a review from a team March 11, 2024 00:32
Copy link
Member

@smebberson smebberson left a comment

Choose a reason for hiding this comment

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

@allanchau, a couple of things to consider.

};

const parseResponse = async (response) => {
response.result = await parseBody(response);
response.result = response.body ? await parseBody(response) : {};
Copy link
Member

Choose a reason for hiding this comment

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

I feel like null would be more appropriate here? An empty object seems to hard to detect and hard to reason about.

Suggested change
response.result = response.body ? await parseBody(response) : {};
response.result = response.body ? await parseBody(response) : null;

Copy link
Member

Choose a reason for hiding this comment

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

@allanchau, this is basically an error in the first version too but I imagine that if you try and stream through binary data (for example, a PDF file) our parsing code would error (specifically the parseBody function because it always assumes the body is text). I guess you could write a unit test to test this?

I think we should update our code to take this opportunity to only parse the responseBody if it exists and if it's a content type we'll support? If it's not, we don't add result to response. If it is, then we attempt to pass it. Something like:

const parseResponse = async (response) => {
    if (response.headers && ['application/json', 'text/'].includes(response.headers.get('Content-Type')) && response.body) {
        response.result = await parseBody(response);
    }

    return response;
}

If we are expecting binary data or something else, then we can handle it as required in our up stream code. Maybe we can add better support for it in the future but I don't know what that looks like at the moment.

Copy link
Member Author

Choose a reason for hiding this comment

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

@smebberson Yeah I'll update it. I had something like this originally, but then realised there was a test that was expecting an empty object.

Copy link
Member Author

Choose a reason for hiding this comment

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

@smebberson I'm not sure using the content-type header will work, by default streams are application/octet-stream which could be anything including json.
What about this code that will error if it can't decode the body and return response.result with the original response body unparsed?
Note the updated TextDecoder options and parseResponse function

const parseBody = async (response) => {
    const decoder = new TextDecoder('utf-8', { fatal: true });
    let body = '';

    for await (const chunk of response.body) {
        body += decoder.decode(chunk);
    }

    if (
        response.headers &&
        response.headers.get('Content-Type') &&
        response.headers.get('Content-Type').includes('application/json') &&
        body.length
    ) {
        return JSON.parse(body);
    }

    return body;
};

const parseResponse = async (response) => {
    if (response.body) {
        try {
            response.result = await parseBody(response);
        } catch (err) {
            console.log(err);
            response.result = response.body;
        }
    }

    return response;
};

packages/fetch/index.js Outdated Show resolved Hide resolved
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.

None yet

2 participants