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

Fix/form data in response #1118

Closed
wants to merge 5 commits into from

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Mar 17, 2021

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain:

What changes did you make? (provide an overview)

  1. Added a test case illustrating an issue
  2. Addressed a bug caused by missed function argument in Response implementation.
  3. Addressed issue with form-data body where boundary fragments are emitted as strings while content fragments as binary data. Which caused exceptions.

Which issue (if any) does this pull request address?

See test case illustrating the issue. It throws exception without this fix.

Is there anything you'd like reviewers to know?

  1. I had to workaround code coverage problem which reported issue for the line https://coveralls.io/builds/38068042/source?filename=src%2Fbody.js#L350 which is unreachable because FormData body is normalized into a stream and I don't believe there are no code paths in which request/response .body could be FormData.

    node-fetch/src/body.js

    Lines 53 to 56 in 1780f5a

    } else if (isFormData(body)) {
    // Body is an instance of formdata-node
    boundary = `NodeFetchFormDataBoundary${getBoundary()}`;
    body = Stream.Readable.from(formDataIterator(body, boundary));

    I do not believe changes here affected code coverage, however instead of removing those lines (which break no tests) I've added a assertion to exercise that code path.

  2. Normalizing FormData to stream during body initialization has a problem of making content length undetectable:

    const totalBytes = getTotalBytes(request);

    And this code path will never be called

    node-fetch/src/body.js

    Lines 352 to 355 in 1780f5a

    // Body is a spec-compliant form-data
    if (isFormData(body)) {
    return getFormDataLength(request[INTERNALS].boundary);
    }

    But I'll defer this to another issue

  3. (Last) commit dc5be05 adds bunch of type annotations which helped me to reason through by getting better IntelliSense as opposed to remembering all the ins and outs. Feel free to get rid of it if you don't want them.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Sep 26, 2021

superseded by #1314
do same thing + adds support for formData decoding

@LinusU
Copy link
Member

LinusU commented Nov 5, 2021

#1314 should be landed soon which fixes this, thank you for taking the time with this ☺️

@LinusU LinusU closed this Nov 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants