fix(FormData): spec compliance

[KhafraDev]

• Fixes argument length being incorrect (very minor)
• Updates makeEntry since the spec was updated apparently.

Surprisingly the FormData class was almost entirely correct and even did most of the webidl conversions.

[codecov-commenter]

Codecov Report

Merging #1501 (55dc797) into main (8924332) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1501   +/-   ##
=======================================
  Coverage   94.83%   94.83%           
=======================================
  Files          50       50           
  Lines        4512     4513    +1     
=======================================
+ Hits         4279     4280    +1     
  Misses        233      233           

Impacted Files	Coverage Δ
lib/fetch/file.js	92.85% <100.00%> (ø)
lib/fetch/formdata.js	96.07% <100.00%> (+0.03%)	⬆️
lib/fetch/webidl.js	99.25% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8924332...55dc797. Read the comment docs.

[ronag]

Surprisingly? 😄

[KhafraDev]

Nothing else does the webidl conversions and FormData's doing all of them other than filename! I was honestly surprised 😄

[jonlil]

I think there is an issue with the changes in the FormData constructor which seems to not be compliant with the webidl spec
https://searchfox.org/mozilla-central/source/dom/webidl/FormData.webidl

The first example of FormData usage on mozila does not work after this change

TypeError: FormData constructor: Argument 1 could not be converted to: null.

NodeJS: v18.6.0

[ronag]

@KhafraDev

[jimmywarting]

Neither NodeJS or web worker have access to DOM, so what is really the problem? you can't construct a formdata with any arguments.

thing it should still say Failed to construct 'FormData': parameter 1 is not of type 'HTMLFormElement'. doe... but it was removed in this commit 😕

[jonlil]

Neither NodeJS or web worker have access to DOM, so what is really the problem

In my specific case we use JSDOM with Vitest to run react unit-test. And since NodeJS 18 ships with FormData (implemented by undici) our tests started failing.

[jimmywarting]

i think perhaps the verdict is that "if you use jsdom" then you should also use jsdom formdata + jsdom fetch instead using undici components

At least that was the jsdom authors arguments when i made a case with the Blob class as being incompatible with each other. it used more strict:er "instanceof OwnComponents" checks... and it stored the data on the blob that wasn't accessible without a jsdom FileReader. and they (still) lacks the new methods to read the blob with any of the promise-methods.

ref: jsdom/jsdom#2555

I think (correct me if i'm wrong) that undici also accepts 3th party instances of FormData too (that aren't instances of undici's FormData) and they comply to the methods of iterating over it and reading each and every entry with iterators and being able to read files (as long as they have the newer read methods)

[Mange]

i think perhaps the verdict is that "if you use jsdom" then you should also use jsdom formdata + jsdom fetch instead using undici components

Would you mind explaining how? I do not wish to use undici, but since Node released with it exposed in the global namespace by default it has just… replaced the other things. Can we stop that from happening somehow?

I suppose this might be a test runner issue. It used to expose FormData but now it doesn't anymore, I would assume because the constant is now present and the runner doesn't want to overwrite anything that is already defined.

Is the intention that FormData in Node will never be spec-compliant? In that case I might petition the testing library to always override it with one that is spec-compliant instead. If not, perhaps they'll need to do some feature probing to check if it should be replaced or not.

---

And for anyone else coming here from Google, I solved it on my case like this:

import { JSDOM } from "jsdom";
const jsdom = JSDOM(`<!DOCTYPE html>`)
const FormData = jsdom.window.FormData;
export FormData;

Then I could import this in my tests to shadow the globally defined FormData.

[jimmywarting]

i think jsdom should actually run in a own separated context like WebWorker instead with it's own sets of global variables

[Mange]

i think jsdom should actually run in a own separated context like WebWorker instead with it's own sets of global variables

The problem is when the implementation code uses FormData. It's part of the JS standard and available everywhere and everything works.

Then you want to write a test for it that runs in a Node test runner. Node places its own constant on top that doesn't behave like the standard.

Take this example:

// feature.ts
export default function someExampleFeature(document: Document) {
  document.addEventListener("submit", (event) => {
    if (event.target instanceof HTMLFormElement) {
      const form = event.target;
      const data = new FormData(form);

      if (data.has("message")) {
        console.log("Form submitted with message:", data.get("message"));
      } else {
        console.log("Submitted a form without a message");
      }
    }
  })
}

// app.ts
import someExampleFeature from "feature";
document.addEventListener("DOMContentLoaded", () => someExampleFeature(document));

Nothing here is strange (assuming I didn't make some unrelated mistake as I'm just typing this in a Github textarea after all).
But if you then want to write a test for this feature, then it will crash when a form is submitted.

You'd have to work around it by adding code to allow injecting of "alternative" implementations of FormData because the one Node provides is not behaving correctly.

  // feature.ts
+ export default function someExampleFeature(document: Document, formData?: any) {
+   const FormDataImpl = formData || FormData;
  
    document.addEventListener("submit", (event) => {
      if (event.target instanceof HTMLFormElement) {
        const form = event.target;
+       const data = new FormDataImpl(form);
  
        if (data.has("message")) {
          console.log("Form submitted with message:", data.get("message"));
        } else {
          console.log("Submitted a form without a message");
        }
      }
    })
  }

[jimmywarting]

Node places its own constant on top that doesn't behave like the standard.

🤔 hmm i don't think those globals are constants, you should be able to override them.

[KhafraDev]

This should be made into an issue on the wintercg repo; is it better to allow a HTMLFormElement look alike or throw an error when an argument is passed to the constructor (current behavior in undici & deno)?

[jimmywarting]

in my point of view i mostly see Deno, cloudflare workers and NodeJS as some sort of web worker that don't have access to the DOM apis
so i don't think it need support for that. I think it should just throw an error (as i don't see it as something that is necessary)

if anything then i think FormData should be acceptable of more or less the same arguments URLSearchParam dose with the added addition of blob support so that you can easily convert URLSearchParams & FormData back and forth

it's easy to convert a html form to URLSearchParam and using FormData and convert it to a GET request with search params

fetch(`/?${new URLSearchParams( new FormData( document.querySelector('form') ) )}`)
// but the revers is not easy for URLSearchParams back into a FormData
new FormData(new URLSearchParams({ foo: 'bar' })) // error not a HTMLFormElement

If anything FormData should have been decoupled from any DOM api all together and don't have anything to do with it. since FormData can also live in Web Workers where the DOM is not accessible.
we should instead have come up with something like:

const iterable = document.querySelector('form').values() // yields [string, string | File] for each element

new URLSearchParams( iterable )

new FormData( iterable )

new FormData({
  foo: 'bar',
  baz: new Blob()
})

new FormData([
  ['foo', 'bar'],
  ['baz', new Blob()],
  ['baz', new Blob()]
])

but it is easy to be a hindsight...