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

UploadFile() should accept Readable instead of Buffer #13158

Open
1 task done
samchon opened this issue Feb 2, 2024 · 9 comments
Open
1 task done

UploadFile() should accept Readable instead of Buffer #13158

samchon opened this issue Feb 2, 2024 · 9 comments

Comments

@samchon
Copy link

samchon commented Feb 2, 2024

Is there an existing issue that is already proposing this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe it

When using @UploadFile() decorator following the NestJS guide documents, it becomes Buffer type.

As you know, the Buffer type means that backend server gathers entire data of the uploaded file. If client uploads 1GB file through the API, the NestJS backend server requires at least 1GB memory about the request. If there're 100 clients uploading the 1GB file at the same time, NestJS backend server needs 100GB memory at that time.

Describe the solution you'd like

I know that the default option of multer is using the Buffer type instance, but it seems not proper to the full framework like NestJS. If configure storage engine of the multer, the uploadaed file would be Readable stream type, and I think NestJS should do it.

Teachability, documentation, adoption, migration strategy

I have followed the https://docs.nestjs.com/techniques/file-upload document.

And very surprised by too much memory consumption.

What is the motivation / use case for changing the behavior?

I think NestJS should do like below:

  1. Make streaming to be default
  2. When some users want Buffer feature, they must configure by themselves
  3. If hard to migrate, then have to warn those facts in the guide documents
@samchon samchon added needs triage This issue has not been looked into type: enhancement 🐺 labels Feb 2, 2024
@jmcdo29
Copy link
Member

jmcdo29 commented Feb 2, 2024

The thing is, it can accept Readable as you mentioned, with the right configuration. While it may be a better solution, in terms of memory management, it's also a more advanced solution in terms of Node API knowledge as not everyone is well versed with streams, and it's a breaking change at the moment. We'd have to make sure to update the docs to show how the stream could be worked with, and probably be ready to answer many, many more questions on how to use the newer approach, which would inevitably be sent to Discord or StackOverflow and answered there.

While I do think that better memory management is a good thing, there is a balance to be kept between efficient and ergonomic API for developers.

@jmcdo29
Copy link
Member

jmcdo29 commented Feb 2, 2024

Perhaps we can make a mention in the docs and the default of buffer and how to use streams if that is the desired approach and why you might want to do that

@samchon
Copy link
Author

samchon commented Feb 2, 2024

If hope to avoid breaking change, I think NestJS guide document must warn the memory problem, and give an example code converting to the streaming strategy.

@kamilmysliwiec
Copy link
Member

Adding a mention in the docs sounds valuable

@micalevisk micalevisk removed the needs triage This issue has not been looked into label Feb 2, 2024
@samchon
Copy link
Author

samchon commented Feb 8, 2024

I'd tried to make concise @UploadFile() similar decorator supports both express and fastify, and understood why @kamilmysliwiec and @jmcdo29 had configured of @UploadFile() to just utilize the full Buffer data as default. To perform streaming, have to configure storage engine of multer, but it was not easy to determine which option to adjust as default.

On the other hand, fastify's multipart/form-data was much easier to handle. This is because streaming could be used right away without any additional settings. Therefore, I had developed @TypedFormData.Body() to utillize the full data Buffer to compatible with both express and fastify, and planning to apply memory storage to the multer side by referring to fastify's options.

Anyway, after I completed this @TypedFormData.Body() and studied it thoroughly through various experiments, I will contribute to Nest Document. The next PR may be the next month.

@Shandur
Copy link

Shandur commented Jul 24, 2024

@samchon Have you had any chance to contribute to NestJS documentation? :)

@samchon
Copy link
Author

samchon commented Jul 24, 2024

Have forgotten this issue for a long time due to busy.

@Shandur Thanks for reminding. It's okay you to contribute eariler.

@saram-aman
Copy link

@samchon is the issue still in the queue or it has been closed?, if workable, assign it to me!

Thanks!

@samchon
Copy link
Author

samchon commented Dec 4, 2024

I'm not maintainer of NestJS, so that as you wish @saram-aman

In my case, I've solved this problem just by defining multer instance by closure function parameter like below.

https://nestia.io/docs/core/TypedFormData/

import { TypedFormData, TypedRoute } from "@nestia/core";
import { Controller } from "@nestjs/common";
import Multer from "multer";
 
@Controller("bbs/articles")
export class BbsArticlesController {
  @TypedRoute.Post()
  public async create(
    @TypedFormData.Body(() => Multer({
      storage: { ...THE STORAGE ENGINE SETTING }
    })) input: IBbsArticleCreate,
  ): Promise<void> {
    input;
  }
}
 
export interface IBbsArticleCreate {
  title: string;
  body: string | null;
  thumbnail?: File | undefined;
  files: File[];
  tags: string[];
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants