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

Multiple files upload with addToBody stores only one file #64

Closed
SkeLLLa opened this issue Apr 17, 2019 · 10 comments
Closed

Multiple files upload with addToBody stores only one file #64

SkeLLLa opened this issue Apr 17, 2019 · 10 comments

Comments

@SkeLLLa
Copy link
Contributor

SkeLLLa commented Apr 17, 2019

Hi. As far as can see from the code and examples if multiple files will be uploaded with singe file input, it will put them into the same field in body and filename and other fields inside it will correspond to one of the files.

Multiple files could be tracked by number of calls of defaultConsumer or if that function is called with different filename. So I think it's better to track it by call number which will represent an index in array. According to this field type in body could be also an array of items with sharedSchemaId type. And if that array has only one file, then, probably, it could replace array, so we'll get an file object as it's done right now.

@mcollina
Copy link
Member

Definitely, would you like to send a PR?

@SkeLLLa
Copy link
Contributor Author

SkeLLLa commented Apr 18, 2019

Yes I can, probably in couple of days if someone will not make it faster.

But I have a question regarding filed type. If we have input with name file should the key file in body to be an array even if there's only one file in there. Or should we put an object instead? E.g.

body = {
  file: [{name, data, ...etc}]
}

or

body = {
  file: {name, data, ...etc}
}

Also I'm thinking about an idea of changing the default behaviour of this module. I think it would be better if this module will use addToBody behaviour as default one. It will be easier to use it and it will cover most of user usecases. Also it will make code look prettier, because all the stuff with event listeners, onFinish handlers will be done under the hood.

And if someone will need "streams API" I think it can be done in the same manner. But in field file for example we'll have and array of object where data will be a readable stream instead of buffer. And we'll add, for example, extra filed to request object that will be a promise, which can be awaited to ensure that all streams were read.

That's just some thoughts, that I think would be nice to have to release version 1.0.0.
WDYT, @mcollina.

@SkeLLLa
Copy link
Contributor Author

SkeLLLa commented Apr 18, 2019

I've made some progress regarding the PR, but I'm stuck in writing tests, because it seems that form-data doesn't support multiple values in same field, like browser does.

@nwoltman
Copy link

@SkeLLLa In what way does it not support multiple values? Appending to the same field should work.

form.append('upload', file1)
form.append('upload', file2)

@SkeLLLa
Copy link
Contributor Author

SkeLLLa commented Apr 19, 2019

@nwoltman I've tried such approach, but in that way I receive only file2 in upload field. Maybe the bug is somewhere in my changes, but when I use html form from browser all is working fine.

UPD: I've got what was wrong. I've tried to add the same file multiple times, that's why I received only one file in the end.

@Eomm
Copy link
Member

Eomm commented Apr 19, 2019

should the key file in body to be an array even if there's only one file in there. Or should we put an object instead?

I think that it should if we don't want to release a semver-major change.
I would like to keep the behaviour of one file = one field in the body and not an array (and multiple files in one field will be an array)

I don't know if this behaviour could be confusing for users, @mcollina WDYT?

@SkeLLLa
Copy link
Contributor Author

SkeLLLa commented Apr 19, 2019

Actually both options could be a bit confusing. If we have, for example one file = object, multiple files = array of objects, then it will need additional if statements on server side to check how many files were uploaded if there's input with multiple file upload enabled. On the other hand if we have an array of files in all cases then it will be a bit weird to have an array there if there is single file input.

@mcollina
Copy link
Member

I’m ok in changing addToBody to set an array of files, it’s a pretty new feature and we won’t break much. I might be ok in making addToBody the default. I’m not convinced in changes to the other API.

@nwoltman
Copy link

@SkeLLLa The solution to that would be to allow each route to configure how many files it expects for each specific file field. This is what multer does.

@Eomm
Copy link
Member

Eomm commented May 22, 2019

released in v1.0.0 👍

@Eomm Eomm closed this as completed May 22, 2019
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

No branches or pull requests

4 participants