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

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

Open
2 tasks done
dzek69 opened this issue Oct 11, 2024 · 7 comments

Comments

@dzek69
Copy link

dzek69 commented Oct 11, 2024

Prerequisites

  • I have written a descriptive issue title
  • 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

@mcollina
Copy link
Member

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

@dzek69
Copy link
Author

dzek69 commented Oct 12, 2024

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

@mcollina
Copy link
Member

I think we can change this to:

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

@dzek69
Copy link
Author

dzek69 commented Oct 15, 2024

Are you fine with breaking changes?

@mcollina
Copy link
Member

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

@dzek69
Copy link
Author

dzek69 commented Oct 15, 2024

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

@mcollina
Copy link
Member

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

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

2 participants