w3ctag / design-reviews

W3C specs and API reviews
Creative Commons Zero v1.0 Universal
322 stars 55 forks source link

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

Closed jenseng closed 1 year ago

jenseng commented 1 year ago

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 commented 1 year ago

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: https://github.com/w3ctag/design-principles/issues/366 (consensus on the principle, but I still need to write it)

jenseng commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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! :sparkles: /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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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.

annevk commented 1 year ago

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.