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: add FormData to URLSearchParams constructor #880

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

keithamus
Copy link
Member

See microsoft/TypeScript#30584

I'm aware that URLSearchParams also accepts generic iterators (see #741) but mostly interested in getting this specific pattern fixed quickly, as it's used a lot.

@keithamus keithamus requested a review from sandersn as a code owner July 1, 2020 09:53
@niklasf
Copy link

niklasf commented Sep 8, 2020

FormData can look like Record<string, string | File> (spec), not just Record<string, string>, so adding this is not valid in general.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

  1. Add to overridingTypes.json instead of the *.generated files.
  2. @niklasf points out that FormData might not work when it contains a File. What happens in this case?
  3. Playing around, @orta and I couldn't get reasonable output from any FormData. Are we doing something wrong?
> const f = document.createElement("form")
< undefined
> const fdd = new FormData(f, { hi: "string" })
< undefined
> fdd
< FormData {append: function, delete: function, get: function, getAll: function, has: function, …}
> new URLSearchParams(fdd).toString()
> ""

@WesleySmits
Copy link

@sandersn In your example the form doesn't have any form field values to map to the URLSearchParams.
See the following example:

const form = document.createElement('FORM');
const field = document.createElement('input');
field.name = 'fieldName';
field.value = 'fieldValue';

form.appendChild(field);

const formData = new FormData(form);
const urlSearchParams = new URLSearchParams(formData);
urlSearchParams.toString(); // "fieldName=fieldValue"

@saschanaz
Copy link
Contributor

Shorter:

const f = new FormData();
f.append('foo', 'bar');
new URLSearchParams(f).toString(); // foo=bar

In the case of blobs it does still work, but in broken way:

const f = new FormData();
f.append('foo', new Blob(['bar']));
new URLSearchParams(f).toString(); // foo=%5Bobject+File%5D

@stefanprobst
Copy link

it would be great to get this in, as this pattern is quite prominently referred to already in the third sentence in the mdn entry on FormData.

@niklasf
Copy link

niklasf commented Oct 20, 2020

Allowing this without a type assertion would be somewhat inconsistent. It's like changing

interface URLSearchParams {
   set(name: str, value: str)
}

to

interface URLSearchParams {
   set(name: str, value: any /* !* /)
}

just because any value will be coerced to a string, like {} is coerced to [object Object], or a file is coerced to [object File] as saschanaz found.

@saschanaz
Copy link
Contributor

One way could be making FormData generic. FormData<string> should be always safe then.

@dylhack
Copy link

dylhack commented Feb 22, 2024

Any updates on this?

@arty-name
Copy link

arty-name commented Oct 28, 2024

I’d argue that having file inputs represented as [object File] is somewhat blessed by the HTML specification itself. In other words, the form with a file input but with enctype="application/x-www-form-urlencoded" will submit string [object File] without any JS involved.

@ITenthusiasm
Copy link

@sandersn Regarding your original comment, it seems that the following comments address concerns #2 and #3:

Are you still open to accepting this PR if #1 gets addressed? @keithamus seemed to properly add changes to overridingTypes.json. Or was your desire also for the changes to *.generated.dts to be deleted?

@ITenthusiasm
Copy link

Also happy to take this over (with guidance 😅) if needed. I think this would be a great feature to push through. I currently need it for some Next.js work.

@sandersn
Copy link
Member

@ITenthusiasm I looked over the post-2020 discussion on the TS bug and I don't think this is the correct change. See my comment on microsoft/TypeScript#30584 for details.

@ITenthusiasm
Copy link

@sandersn Understood. Thanks for taking the time to review/reconsider the PR and the linked issue! Is there any hope of microsoft/TypeScript#43797 being seen as a viable option at some point? (Just trying to set expectations for the FormData + URLSearchParams concern.)

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.

9 participants