feat: add option to store files

[leonardovillela]

Description

This PR introduces changes to support stores in the host machine files uploaded. The new option storeFiles with default value as true determines it, if true the files are parsed and stored, if false the files are only parsed. Closes #609.

Key Points

• I tried to maintain this retro compatible and not introduce any breaking change.
• I thought to have a class for the base file to avoid duplication of code between VolatileFile and PersistentFile, but in the end, I thought it was not worthy. Let me know what you think about it.
• I focused more on integration tests for this PR because I think it maps better for this type of change, but let me know if I need to add more unit tests.

[codecov]

Codecov Report

Merging #666 (21d62c1) into master (76ad4ae) will decrease coverage by 0.69%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
- Coverage   82.05%   81.35%   -0.70%     
==========================================
  Files          14       15       +1     
  Lines         652      692      +40     
  Branches      108      114       +6     
==========================================
+ Hits          535      563      +28     
- Misses        109      120      +11     
- Partials        8        9       +1     

Impacted Files	Coverage Δ
src/PersistentFile.js	80.48% <66.66%> (ø)
src/VolatileFile.js	67.56% <67.56%> (ø)
src/Formidable.js	72.85% <71.42%> (+0.24%)	⬆️
src/index.js	100.00% <100.00%> (ø)
src/plugins/octetstream.js	94.11% <100.00%> (-0.17%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76ad4ae...21d62c1. Read the comment docs.

[GrosSacASac]

Could you make an example that shows how to use volatile file as an input to another http request body ?

[tunnckoCore]

Haha, seems like a clever approach.

The only thing that bugs me every time and constantly forgetting "what's the difference between filename and name, what's the diff between type and mime? Is there a difference?". I think we should get rid of them, only filename and mime to remain? But that's probably for later, still for v2 tho.

[tunnckoCore]

Probably add a hint in the features section where we say "automatically writes to disk (optional soon)", something like "see options.storeFiles?

[leonardovillela]

Could you make an example that shows how to use volatile file as an input to another http request body ?

@GrosSacASac not sure if I understand correctly what to do, it's to me add on README an example of usage of formidable to only parse files and store It?

[leonardovillela]

The only thing that bugs me every time and constantly forgetting "what's the difference between filename and name, what's the diff between type and mime? Is there a difference?". I think we should get rid of them, only filename and mime to remain? But that's probably for later, still for v2 tho.

@tunnckoCore I think they all represent the same things, so there is no difference. But I think we should not change the code to have the same nomenclature in all places, because in the context of an HTTP request the mime and filename nomenclature makes more sense than type and name, and in the context of file mime don't make sense because it's related to HTTP request context(a.k.a domain) and the filename sound a bit repetitive because we already in the context of a file.

[GrosSacASac]

Readme or in examples https://github.com/node-formidable/formidable/tree/master/examples

[leonardovillela]

@GrosSacASac and @tunnckoCore request changes are done :)

[GrosSacASac]

How do you redirect the stream to something else other than a file ? With volatilefile ? How to overwrite the write () {} ? Or did I miss something.
The example does not show how.

[tunnckoCore]

How do you redirect the stream to something else other than a file ? With volatilefile ? How to overwrite the write () {} ? Or did I miss something.
The example does not show how.

Not sure what you mean.

You can get the file from { files } then redirect it wherever you want? That's what I understand, i think 🤔

[GrosSacASac]

{ files } only contain VolatileFile instances, which do not contain the file content itself (write method never saves it).

[GrosSacASac]

To be clear: There are 2 thing this PR should do:

1. An option to not store file in the fs (done)
2. An option to do something else with the file (I don't see how in the current PR)

{ files } only contains file size and file names.

May be add another option besides storeFiles

• handleFileStream(name, stream)

[GrosSacASac]

Or we can merge this now, and make another PR for part 2

[tunnckoCore]

Oh, I got it. Fair point.

I think it would be better as part of this PR... I don't know.

Merry Christmas 😉

[leonardovillela]

Hey, @tunnckoCore and @GrosSacASac sorry for the late response.

I added changes to write the file to something else(cloud storages, stdout and etc...) I also added two examples of how to do this. I opted to maintain only one option because I don't see much value in having two options that are coupled.

BTW the integration test is broken on CI, but on my machine, they are passing, I tried to fix this but I can't 😞 , can you help me? Fixed 😄

[GrosSacASac]

Well done

[Benjy96]

Was this functionality removed?

[GrosSacASac]

No