node-formidable / formidable

The most used, flexible, fast and streaming parser for multipart form data. Supports uploading to serverless environments, AWS S3, Azure, GCP or the filesystem. Used in production.
MIT License
7k stars 680 forks source link

default treament for array like forms with signifier "[]" #902

Closed captainhusaynpenguin closed 1 year ago

captainhusaynpenguin commented 1 year ago

In my experience, using "[]" as signifier for a json like array in HTML has been a standard practice in PHP and Ruby, and the reason I choose formidable. That we are choosing to drop that for version 3 is quite questionable ...

Every is for sure entitled to their hatred as by https://github.com/node-formidable/formidable/issues/634, but I really think it is going to be a missing essential feature.

GrosSacASac commented 1 year ago

[] still works in v3 as everything is an array

captainhusaynpenguin commented 1 year ago

here is the code:

import express from 'express';
import formidable from 'formidable';

const app = express();

app.get('/', (req, res) => {
  res.send(`
    <h2>With <code>"express"</code> npm package</h2>
    <form action="/api/upload" enctype="multipart/form-data" method="post">
      <div>Text field title: <input type="text" name="name[first]" /></div>
      <div>Text field title: <input type="text" name="name[last]" /></div>
      <div>Text field title: <input type="text" name="name[nick]" /></div>
      <div>File: <input type="file" name="someExpressFiles" multiple="multiple" /></div>
      <input type="submit" value="Upload" />
    </form>
  `);
});

app.post('/api/upload', (req, res, next) => {
  const form = formidable({ multiples: true });

  form.parse(req, (err, fields, files) => {
    if (err) {
      next(err);
      return;
    }
    res.json({ fields, files });
  });
});

app.listen(3000, () => {
  console.log('Server listening on http://localhost:3000 ...');
});

result from version 2.0.1:

{"fields":{"name":{"first":"Lionel","last":"Messi","nick":"Leo"}},"files":{"someExpressFiles":{"size":5493,"filepath":"/tmp/49274a379616e78eb4e00c800","newFilename":"49274a379616e78eb4e00c800","mimetype":"image/jpeg","mtime":"2022-11-12T20:13:00.555Z","originalFilename":"download.jpg"}}}

and the result from version 3.2.5:

{"fields":{"name[first]":["First Name"],"name[last]":["Last Name"],"name[nick]":["Nickname"]},"files":{"someExpressFiles":[{"size":5493,"filepath":"/tmp/fcbea8b6a272c39235186c000","newFilename":"fcbea8b6a272c39235186c000","mimetype":"image/jpeg","mtime":"2022-11-12T20:11:55.927Z","originalFilename":"download.jpg"}]}}

I think believe the first one is more elegant, or in other words, plug & play on the receiving end. Imagine you have a complex form with multiple nested arrays/objects, then you have perform the stuff on your code base ... it's not a huge deal, but keeping al the parsing data from the from logic in a separate library is the goal we are going for, I imagine.

GrosSacASac commented 1 year ago

Communication would be easier if you didn't confuse arrays over objects.

That said, eveything can stil be done post form.parse with a simple function call, or by sending JSON to begin with.

captainhusaynpenguin commented 1 year ago

Sorry, I'm still in PHP mentality, arrays having "key" "value" style with the option to define stings as keys. https://www.php.net/manual/en/language.types.array.php

PS. Appreciate an example how should we/I go about it? i.e. without adding JS to the front-end for mocking form handling.

version 2, even processes something like this correctly:

      <div>Text field title: <input type="text" name="address[city]" /></div>
      <div>Text field title: <input type="text" name="address[street][name]" /></div>
      <div>Text field title: <input type="text" name="address[street][number]" /></div>
{
address: {
  city: .....,
  street: {
    name: .......,
    number: .....,
  }
}
tunnckoCore commented 1 year ago

@captainhusaynpenguin

@GrosSacASac That said, eveything can stil be done post form.parse with a simple function call, or by sending JSON to begin with.

I thought that way initially too. But I also understand why it makes sense to be inside, he don't want (and probably others won't) to do additional parsing.

We can make a helper package for this (well, it's just qs/querystring after form.parse), as we move to smaller and minimal core and additional packages for other things. Because in reality, the whole thing around the parser (with modern native Web APIs like File and FormData) could be done in 100-200 lines, and I actually did it (formidable-mini but i don't remember where the code is) and most tests were passing if i remember correctly (at least was fast as alternatives, in some cases faster).

But the current v3 behavior is actually a standard thing with FormData too. Check this out: https://jsbin.com/zazosoyuta/6/edit?js%2Cconsole%2Coutput=

"address[city]: Foobie"
"address[street][name]: Barry"
"address[street][number]: 124"

Before, we were using the querystring package.. or qs... why I never remembered which one was the core module and which the npm package.. :laughing: People still can use it after the parsing of form.parse.

So... :man_shrugging:

Do you really need to have the data that way. It most probably will be used/showed combined a), and b) even if it's not if you write it together on some DB, you can then easily use something like fuzzy search when needed to get what's needed to be shown or searched. Well... yea, it depends. It always depends on case :laughing:

jimmywarting commented 1 year ago

I was used to php too a long time ago (10y+)

anyways the new way of returning an array would be to use the same name

<input type="hidden" value="delete" name="action" /> 
<input type="checkbox" value="203" name="blogId" />
<input type="checkbox" value="204" name="blogId" />
<input type="checkbox" value="205" name="blogId" />
<input type="file" name="fileUpload">

and then the new way of parsing it would be without any package and actually start using request.formData() instead. kind of like this psudo code:

app.post('/api/upload', async (req, res, next) => {
  const formData = await new Response(req, { headers: req.headers }).formData()
  formData.getAll('blogId') // returns an array of all blob ids to delete
  formData.get('action') // returns "delete"
  formData.get('fileUpload') // returns a File
})

i belive this ☝️ will be the new standard way of posting/parsing stuff

https://twitter.com/3a1fcBx0/status/1601163191589163008

GrosSacASac commented 1 year ago

So PR welcome if someone wants to add an optional helper function.