feat: dropping support for non-native FormData implementations #199

erunion · 2023-09-14T06:24:47Z

🧰 Changes

Now that Node 16 is EOL'd and native fetch and FormData are available everywhere this library needs to run we can now drop support for the legacy, non-native FormData handling we had for the form-data library.

Late last week I packed up some undici, the library that Node 18+ uses for native fetch, internals into a new package named formdata-to-string that we can use to easily transform a FormData object into a payload string. The unfortunate result of this work is that because that library is async then this library also needs to be async.

import { HTTPSnippet } from 'httpsnippet';

const snippet = new HTTPSnippet({
  method: 'GET',
  url: 'httsp://httpbin.org/anything',
});

console.log(await snippet.convert('node'));

The main benefit of this new native FormData work however is that we can delete a lot of code I had written two years ago here and upstream for handling these funky non-native FormData instances that the form-data library, and node-fetch@2 utilized.

And because this work fundamentally changes how the library works I am not planning on upstreaming it.

erunion · 2023-09-14T06:47:21Z

src/fixtures/runCustomFixtures.ts

@@ -24,24 +24,25 @@ export interface CustomFixture {

 export const runCustomFixtures = ({ targetId, clientId, tests }: CustomFixture) => {
  describe(`custom fixtures for ${targetId}:${clientId}`, () => {
-    tests.forEach(({ it: title, expected: fixtureFile, options, input: request }) => {


Had to restructure all of these tests into formal describe.each and it.each blocks because with the async change Vitest wasn't picking them up as they were getting invoked after Vitest was expecting them to because it was running an async test inside of a non-async foreach loop.

erunion · 2023-09-14T06:47:56Z

src/helpers/form-data.ts

Extracted this from node-fetch core a couple years ago when I overhauled the multpart handling in this library. Really glad I can now delete this crap.

erunion · 2023-09-14T06:49:16Z

src/index.ts

-              form.append(name, value, {
-                // @ts-expect-error TODO
-                filename,
-                // @ts-expect-error TODO
-                contentType: param.contentType || null,
-              });


hit da bricks non-native formdata

erunion · 2023-09-14T06:52:44Z

src/targets/c/libcurl/fixtures/multipart-data.c

@@ -8,6 +8,6 @@ struct curl_slist *headers = NULL;
 headers = curl_slist_append(headers, "content-type: multipart/form-data; boundary=---011000010111000001101001");
 curl_easy_setopt(hnd, CURLOPT_HTTPHEADER, headers);

-curl_easy_setopt(hnd, CURLOPT_POSTFIELDS, "-----011000010111000001101001\r\nContent-Disposition: form-data; name=\"foo\"; filename=\"hello.txt\"\r\nContent-Type: text/plain\r\n\r\nHello World\r\n-----011000010111000001101001\r\nContent-Disposition: form-data; name=\"bar\"\r\n\r\nBonjour le monde\r\n-----011000010111000001101001--\r\n");


Fun side effect of the quirky non-native FormData work we had set up was that sometimes the filename in a HAR would get to basenemed from src/fixtures/files/hello.txt to hello.txt. This isn't happening anymore anywhere, and I think that's okay. The HARs we ingest for this library in the browser will always have a basenamed filename thanks to browser security not giving us full paths.

It's also good, as far as integration testing goes, for these filenames to be the full path in snippets everywhere so when we execute these snippets in the Docker integrations suite it's able to send along the file when necessary (like some JS snippets in here that use fs.readFileStream()).

I checked the golang source to confirm that the trailing newlines are not required, and it agrees

erunion · 2023-09-14T06:53:48Z

src/targets/c/libcurl/fixtures/multipart-form-data.c

@@ -8,6 +8,6 @@ struct curl_slist *headers = NULL;
 headers = curl_slist_append(headers, "Content-Type: multipart/form-data; boundary=---011000010111000001101001");
 curl_easy_setopt(hnd, CURLOPT_HTTPHEADER, headers);

-curl_easy_setopt(hnd, CURLOPT_POSTFIELDS, "-----011000010111000001101001\r\nContent-Disposition: form-data; name=\"foo\"\r\n\r\nbar\r\n-----011000010111000001101001--\r\n");


These multipart payloads no longer have trailing \r\n's and that's also okay.

erunion · 2023-09-14T06:55:41Z

src/index.ts

          request.postData.boundary = boundary;
+          request.postData.text = await formDataToString(form).then(str => {


This formdata-to-string library I published last week from extracted undici code is a goddamn dream. Give it a FormData object and it just works without any fuss.

It'd be cool if this library accepted an optional flag where you could say whether your not you want the undici-specific boundary, feels a li'l weird to be doing this here

Wonder why undici doesnt publish that as a util function.

I'm not sure but if they did publish it we'd be importing a whole bunch of junk as their core function has handling for all sorts of body payloads that we don't need to worry about. https://github.com/nodejs/undici/blob/e39a6324c4474c6614cac98b8668e3d036aa6b18/lib/fetch/body.js

@kanadgupta I added a custom boundary option in v1.1.0.

domharrington

LGTM! This is a breaking change right?

src/index.ts

domharrington · 2023-09-14T11:14:12Z

src/index.ts

-          // @ts-expect-error TODO
-          const isNativeFormData = typeof form[Symbol.iterator] === 'function';
-
-          // TODO: THIS ABSOLUTELY MUST BE REMOVED.


😬 good job we're getting rid of this.

domharrington · 2023-09-14T11:14:58Z

src/index.ts

          request.postData.boundary = boundary;
+          request.postData.text = await formDataToString(form).then(str => {


Wonder why undici doesnt publish that as a util function.

kanadgupta

Mostly small comments, LGTM! Even though there's as chance they'll decline, I wouldn't be opposed to offering this to the upstream fork since you're basically a maintainer at this point lol (and mostly everything you're doing here isn't all that controversial, plus it contains a lot of great modernizations)

kanadgupta · 2023-09-14T10:06:00Z

.vscode/settings.json

Is this file here because they use it upstream? This feels like something that should just be in the prettier config

yeah these snippets have all sorts of frustrating line endings, that are handled in .editorconfig, that i fought the upstream maintainers on but they didn't relent.

package.json

src/index.ts

kanadgupta · 2023-09-14T10:13:51Z

src/index.ts

          request.postData.boundary = boundary;
+          request.postData.text = await formDataToString(form).then(str => {


It'd be cool if this library accepted an optional flag where you could say whether your not you want the undici-specific boundary, feels a li'l weird to be doing this here

kanadgupta · 2023-09-14T13:28:26Z

src/index.ts

-          // easter egg
-          const boundary = '---011000010111000001101001'; // this is binary for "api". yep.


:buzzfeed-cute: TIL!

src/index.ts

llimllib

This PR doesn't bump the version in package.json - should this be v8?

domharrington · 2023-09-14T14:59:47Z

This PR doesn't bump the version in package.json - should this be v8?

It should do when merged in! Normally you'd do npm version <patch|minor|major> only when the code is ready to go and then it'll do the package.json bump, git tag and npm publish all at once.

Co-authored-by: Kanad Gupta <[email protected]>

## 🧰 Changes In September of last year I reworked this library[^1] off `node-fetch` and the funky non-native `FormData` object it ships to only support native `FormData` -- in the process making code snippet generation asynchronous. Unfortunately this async work is causing us some headaches on integrating it into our API Explorer. I started to dig in to trying to make `formdata-to-string` synchronous and realized... why bother? We have the HAR, we can generate everything we need for a multipart boundary from that. * [x] Completely removes the `FormData` and `Blob` API dependencies from this library. * [x] Made `.convert()` a synchronous operation again. [^1]: #199 --------- Co-authored-by: Kanad Gupta <[email protected]>

erunion added 2 commits September 13, 2023 21:33

refactor: making the library async

a080016

feat: dropping support for non-native FormData implementations

f99294b

erunion commented Sep 14, 2023

View reviewed changes

erunion added the enhancement New feature or request label Sep 14, 2023

erunion commented Sep 14, 2023

View reviewed changes

erunion marked this pull request as ready for review September 14, 2023 07:11

erunion requested review from llimllib, a team, Dashron, mosesj-readme, domharrington and kanadgupta and removed request for a team September 14, 2023 07:11

domharrington approved these changes Sep 14, 2023

View reviewed changes

kanadgupta approved these changes Sep 14, 2023

View reviewed changes

llimllib reviewed Sep 14, 2023

View reviewed changes

erunion and others added 4 commits September 14, 2023 09:18

fix: making convert async instead of having to call .init()

b70def1

chore: running prettier on everything

87a0fb1

feat: pr feedback

65843b2

Update src/index.ts

5083a85

Co-authored-by: Kanad Gupta <[email protected]>

erunion merged commit aae6c60 into main Sep 14, 2023

erunion deleted the feat/native-fetch branch September 14, 2023 16:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: dropping support for non-native FormData implementations #199

feat: dropping support for non-native FormData implementations #199

erunion commented Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023

erunion Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023 •

edited

Loading

llimllib Sep 14, 2023

erunion Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023

kanadgupta Sep 14, 2023

domharrington Sep 14, 2023

erunion Sep 14, 2023

erunion Sep 14, 2023

domharrington left a comment •

edited

Loading

domharrington Sep 14, 2023

domharrington Sep 14, 2023

kanadgupta left a comment •

edited

Loading

kanadgupta Sep 14, 2023

erunion Sep 14, 2023

kanadgupta Sep 14, 2023

kanadgupta Sep 14, 2023

llimllib left a comment

domharrington commented Sep 14, 2023

		request.postData.boundary = boundary;
		request.postData.text = await formDataToString(form).then(str => {

		// easter egg
		const boundary = '---011000010111000001101001'; // this is binary for "api". yep.

feat: dropping support for non-native FormData implementations #199

feat: dropping support for non-native FormData implementations #199

Conversation

erunion commented Sep 14, 2023 • edited Loading

🧰 Changes

Choose a reason for hiding this comment

erunion Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

erunion Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

erunion Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erunion Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domharrington left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanadgupta left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llimllib left a comment

Choose a reason for hiding this comment

domharrington commented Sep 14, 2023

erunion commented Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023 •

edited

Loading

erunion Sep 14, 2023 •

edited

Loading

domharrington left a comment •

edited

Loading

kanadgupta left a comment •

edited

Loading