fastify-multipart icon indicating copy to clipboard operation
fastify-multipart copied to clipboard

`saveRequestFiles` is consuming value fields and give no way to retreive them if there were no files

Open dzek69 opened this issue 1 year ago • 7 comments

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

3.20.x

Plugin version

6.0.1

Node.js version

21.7.1

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Alpine 3.15 I think

Description

I have a endpoint, where I process optional files. I really like the convenience of saveRequestFiles, but there is one problem - if there were no files - there is no way to access fields (I don't want to use attachFieldsToBody btw) - they are already consumed.

    // this prints "start" and "end" only

    const files = await req.saveRequestFiles();
    // `files` is  an empty array
    console.log("start");
    for await (const part of parts) {
        console.log(part);
    }
    console.log("end");

I know I can still do stuff manually, but maybe there is a way to improve saveRequestFiles somehow? Even "dumb" stuff like attaching extra fields property directly to an array returned by saveRequestFiles() would be nice (that would be non-breaking change)

Link to code that reproduces the bug

No response

Expected Behavior

No response

dzek69 avatar Oct 11 '24 21:10 dzek69

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

mcollina avatar Oct 12 '24 11:10 mcollina

@mcollina I may implement it, but do you agree with proposed solution (adding fields property to an array, which is not the best solution - not future proof in case fields would be added into array prototype) or do you have any better idea?

Another non-breaking solution would be to add another function like saveRequestedFiles, that would return the list of saved files and value fields separately. But there are many functions already (parts(), files(), file(), etc), which is already confusing so i would not do that.

Yet another non breaking solution is to add an param to saveRequestedFiles that would change the return data, but that's not the most beautiful API (but fortunately can be typed with TypeScript).

Do you have any idea maybe?

dzek69 avatar Oct 12 '24 17:10 dzek69

I think we can change this to:

    const { files, values } = await req.saveRequestFiles();

mcollina avatar Oct 15 '24 12:10 mcollina

Are you fine with breaking changes?

dzek69 avatar Oct 15 '24 12:10 dzek69

Yes, it seems we forgot about values in the design of that API.

mcollina avatar Oct 15 '24 12:10 mcollina

hmm... that means no backport for me, as i'm using a version 6.x so it's compatible with fastify 3.x

dzek69 avatar Oct 15 '24 15:10 dzek69

@dzek69 well, that's well out of support anyway, there would never be a backport for fastify 3.x.

mcollina avatar Oct 15 '24 15:10 mcollina

{ attachFieldsToBody: true } is definitely the way to go.

I don't want to use attachFieldsToBody btw

I'm missing a good reason against using this?

soulseekah avatar Nov 17 '25 21:11 soulseekah