Skip to content

feat(formdata): use formdata-node to handle form data #2154

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

Merged
merged 10 commits into from
Jul 27, 2021
Merged

Conversation

char0n
Copy link
Member

@char0n char0n commented Jul 25, 2021

Notable changes in this PR

  • removed use of isomorphic-form-data library
  • added formdata-node library to handle FormData bodies
  • added form-data-encoder to provide compatibility between formdata-node and node-fetch@2.x
  • introduced browser field mapping for isomorphic use and avoiding bundling unnecessary code
  • installation runtime now requires Node.js >= 12.4.x
  • runtime now requires Node.js >= 12.4.x
  • all these changes will allow to use swagger-client in various additional envs like WebWorker or Realm

Note to runtime requirement: when we migrate to node-fetch@3.x the runtime requirement will be Node.js >= 12.20.x.

This PR is a preparation for using node-fetch@3.x which uses formdata-node for FormData API operations and not the obscure form-data library which is not spec-compliant and causes various issues.

Refs #2008

@char0n
Copy link
Member Author

char0n commented Jul 25, 2021

This PR will fail unless new releases of formdata-node is released with my change. Also formdata-node requires at least Node.js@12.4.x to be installed.

This remove use of isomorphic-form-data.

Refs #2008
@char0n char0n requested a review from tim-lai July 26, 2021 20:31
Copy link
Contributor

@tim-lai tim-lai left a comment

Choose a reason for hiding this comment

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

@char0n Was this tested in (npm link) SwaggerUI and with "Try It Out"? We previously tried to migrate to formdata-node and deprecate use of isomorphic-fetch, but had to roll it back. That said, this PR seems cover additional considerations.

  • Node 12.4 requirement: Ok, no issue from CI perspective.
  • What is preventing use of node-fetch@3 in this PR?

@char0n
Copy link
Member Author

char0n commented Jul 27, 2021

@char0n Was this tested in (npm link) SwaggerUI and with "Try It Out"?

Yes it has. I've tested manually and run SwaggerUI tests on locally linked swagger-client. Technically this PR brings no change to the SwaggerUI. This is due to following reasons:

  • my change that adds isomorphic support for formdata-node is already present in the version of the library this PR introduces
  • browser override in package.json make sure that native FormData instance is passed to native fetch function

We previously tried to migrate to formdata-node and deprecate use of isomorphic-fetch, but had to roll it back. That said, this PR seems cover additional considerations.

Yes additional things needed to be done to successfully integrate formdata-node. Author of the library helped me understand the complexities and reasons between formdata-node vs form-data vs node-fetch (v2 vs v3)

  • Node 12.4 requirement: Ok, no issue from CI perspective.

No issue, minimal version in workflows haves been bumped to 12.4.x

  • What is preventing use of node-fetch@3 in this PR?

node-fetch@3 is still in beta and not ready for production use. I assume they'll release first stable during one or two months.

Co-authored-by: Tim Lai <timothy.lai@gmail.com>
@octet-stream
Copy link

node-fetch@3 is still in beta and not ready for production use. I assume they'll release first stable during one or two months.

Not to mention that since 3.0.0-beta.10 node-fetch requires native ESM support, which means that you can't use it unless you get rid of CJS in your package, or unless you can use dynamic import().

@char0n
Copy link
Member Author

char0n commented Jul 27, 2021

Not to mention that since 3.0.0-beta.10 node-fetch requires native ESM support, which means that you can't use it unless you get rid of CJS in your package, or unless you can use dynamic import().

Yeah, that'll be fun ;] We already use ESM in 99% places and transpile to CJS, so it shouldn't be that much of a hassle.

@char0n
Copy link
Member Author

char0n commented Jul 27, 2021

Ok let's go for a merge and release!

@char0n char0n merged commit 59c4b79 into master Jul 27, 2021
swagger-bot pushed a commit that referenced this pull request Jul 27, 2021
# [3.14.0](v3.13.7...v3.14.0) (2021-07-27)

### Features

* **formdata:** use formdata-node to handle form data ([#2154](#2154)) ([59c4b79](59c4b79)), closes [#2008](#2008)
@swagger-bot
Copy link
Contributor

🎉 This PR is included in version 3.14.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants