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

FYI - Add optional submitter parameter to the FormData constructor #812

Closed
1 task done
jenseng opened this issue Jan 30, 2023 · 8 comments
Closed
1 task done

FYI - Add optional submitter parameter to the FormData constructor #812

jenseng opened this issue Jan 30, 2023 · 8 comments
Assignees
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Venue: WHATWG

Comments

@jenseng
Copy link

jenseng commented Jan 30, 2023

Wotcher TAG!

I'm requesting a TAG review of the optional submitter parameter to the FormData constructor.

Per the Chromium Intent to Ship, it was requested that I file a TAG review as a FYI - hence this request.

This new optional parameter allows developers to construct a FormData object that matches an equivalent native form submission, i.e. including the submit button entry(s), as appropriate.

Further details:

We'd prefer the TAG provide feedback as:

💬 leave review feedback as a comment in this issue and @-notify @jenseng

@LeaVerou
Copy link
Member

From an API design point of view, I find it odd that this was added as a positional argument and not a dictionary parameter. Seeing one of the examples in the wild, I'd have no idea what the second parameter is for, whereas {submitter: myform.querySelector("#unnamed")} is far more clear (and extensible).

See also: w3ctag/design-principles#366 (consensus on the principle, but I still need to write it)

@jenseng
Copy link
Author

jenseng commented Jan 30, 2023

Ooh that's a great point @LeaVerou!

There were discussions around this before I got involved, and the consensus was to add a second optional argument, @annevk might be able to add some more context around those conversations.

I probably should have provided a better example, as it's a little clearer and more ergonomic for the primary use case. Typically this would be used after a user clicks on a submit button, e.g. something like:

myform.addEventListener("submit", e => {
  e.preventDefault();
  const formData = new FormData(e.target, e.submitter);
  // do a fetch or something
});

or

mybutton.addEventListener("click", e => {
  e.preventDefault();
  const formData = new FormData(e.form, e.target);
  // do a fetch or something
});

@annevk
Copy link
Member

annevk commented Jan 31, 2023

If we were to overload it would have to be the first argument (since <form> itself is not the most natural first argument) and it's not clear that a dictionary that only accepts form and submitter is desirable there. A record would make more sense and you cannot have both. So from that perspective I think this is somewhat reasonable as there are no great options.

@LeaVerou LeaVerou added this to the 2023-02-06-vf2f milestone Jan 31, 2023
@LeaVerou
Copy link
Member

Hi @jenseng – thanks for this and thanks for being receptive to my feedback. Just as a matter of process : we don’t really do FYIs. Either there’s something for us to review and feed back on or there isn’t. In this case, it seems like there very much is so we’ll treat this as we would any other review. Thanks! ✨ /cc @chrishtr

since <form> itself is not the most natural first argument

I respectfully disagree; I find <form> a rather natural first argument for a form data object, whereas submitter is much more secondary and non-obvious.

@jenseng
Copy link
Author

jenseng commented Jan 31, 2023

Just as a matter of process : we don’t really do FYIs. Either there’s something for us to review and feed back on or there isn’t.

Thanks for letting me know! I'll pass this along to the Chromium team, as I think they've done similar "FYI" reviews in the past 🙏

I respectfully disagree; I find <form> a rather natural first argument for a form data object, whereas submitter is much more secondary and non-obvious.

While form feels intuitive to me as well, in practice I don't know if that's what developers typically want/expect it to support. I.e. a very common scenario is wanting to construct a FormData object from an object or array, as can be done with UrlSearchParams, e.g. new UrlSearchParams({ some: "value", another: "value"}) (see xhr issue, typical SO post, npm module workaround). In retrospect, perhaps the constructor should have always accepted a sequence/record, and supported forms a different way (e.g. FormData.fromForm(form, submitter).

In light of that, a mild concern I have around the dictionary approach is that developers could see it in the wild and misunderstand its purpose, trying to use it to specify arbitrary entries, e.g. new FormData(undefined, { some: "value", another: "value"}). So while submitter is less obvious than form, it does relate to it strongly, and thus might avoid some ambiguities introduced by a dictionary.

@chrishtr
Copy link

chrishtr commented Feb 3, 2023

Hi @jenseng – thanks for this and thanks for being receptive to my feedback. Just as a matter of process : we don’t really do FYIs. Either there’s something for us to review and feed back on or there isn’t. In this case, it seems like there very much is so we’ll treat this as we would any other review. Thanks! ✨ /cc @chrishtr

The purpose of an FYI is to give the TAG notice that a change is happening that Chromium representatives think is simple and uncontroversial via other signals. But if you happen to notice design issues not already found, that is good and achieves one purpose of an FYI. The alternative would be not to send one at all, which fails to give you that opportunity.

Upgrading this one to a regular TAG review if you think it's warranted is totally fine and makes sense.

@LeaVerou
Copy link
Member

LeaVerou commented Feb 7, 2023

@plinss and I looked at this during a breakout today.

Our thoughts were:

  1. We think a positional argument for submitter is unintuitive. There is nothing that would indicate that this is a submitter argument when looking at calls to this constructor in the wild.
  2. We do agree that a dictionary replacing the first argument would be confusing, though a dictionary for the second argument does not have this issue.
  3. As the SO post, npm module etc indicate, authors often want to convert arbitrary objects to form data. We want to urge the API owners to explore supporting this use case, either by extending the constructor or through a factory method.

We do not see any architectural issues with adding the argument however, and do not have any other concerns beyond this API design issue, so we will go ahead and close this.

@LeaVerou LeaVerou closed this as completed Feb 7, 2023
@LeaVerou LeaVerou added Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes and removed Progress: in progress labels Feb 7, 2023
@annevk
Copy link
Member

annevk commented Feb 7, 2023

Thank you. Perhaps the following was understood, but I wanted to explain the rationale some more just in case:

  1. Passing the dictionary as second argument doesn't have much value as all other use cases would not involve <form>. If that ends up being false we'll definitely reconsider though. Would be easy to support both.
  2. Overloading the first argument with a dictionary could work, but creates a problem if we want to accept object-like structures there, such as IDL's record<>. Which as you note would be a natural next step for this API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Venue: WHATWG
Projects
None yet
Development

No branches or pull requests

4 participants