Skip to content

Conversation

@erunion
Copy link
Member

@erunion erunion commented Aug 5, 2020

  • This fixes the implementation of form-data in multipart/form-data content types in order to accommodate library utilization within the browser where the native FormData component is used.
    • Key differences between form-data and FormData that was causing the library to fail in the browser were two issues:
      • The API for FormData.append has three arguments, and the third should only be present if the second is a Blob or USVString. It is never an object, as form-data requires it to be.
      • FormData.pipe() isn't a function.
  • Fixes a bug in the Node request target that was causing fs.createReadStream pointers to be stringified and unusable.
  • Updates the following targets to prioritize param.fileName if it's present, even if param.value also is:
    • Shell ↠ cURL
    • Node ↠ request
  • Updates the Node request target to prefer single quotes in require statements and request.cookie calls. Since the rest of the snippet already preferred this, this change makes the whole snippet look a touch more cohesive and consistent.

Since I'm not sure on how we can actually test the browser side of things within this library, you can instead test it out with this PR in the Explorer repo: readmeio/api-explorer#867. That is currently loading this PR within the @readme/oas-to-snippet library:

Screen Shot 2020-08-06 at 3 24 39 PM

Copy link

@rafegoldberg rafegoldberg left a comment

Choose a reason for hiding this comment

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

This does not look like it was fun to code, but the code looks amazingly good for what it is/does/has to be. Super useful comments too. (Could still probably use another set of eyes, but I did read through it pretty closely.)

* @param {FormData} form
* @param {string} boundary
*/
module.exports.formDataIterator = function * (form, boundary) {
Copy link

Choose a reason for hiding this comment

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

Oh wow. A generator in action. I think it's the first time I've actually seen one in usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to look up function * to see what that was because I'd also never seen a JS generator in action before.

@erunion erunion added bug Something isn't working enhancement New feature or request labels Aug 7, 2020
Copy link

@dok dok left a comment

Choose a reason for hiding this comment

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

A bit difficult to imagine how this would integrate into the explorer, but the code looks sound!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants