eronka / culero-api

Open source alternative to LinkedIn
https://www.culero.com
13 stars 10 forks source link

fix: dashboard various fixes #78

Closed adelinaenache closed 4 months ago

adelinaenache commented 4 months ago

Fixes #80 Fixes #79 Fixes #74

adelinaenache commented 4 months ago

One general suggestion that I would like you to have is, since we are using prisma, using a DTO for returning data doesn't really make much sense and seems like extra code.

It does make sense to use. DTO because:

  1. Sometimes prisma types are different of what we actually want to send to the frontend (and sometimes include extra post processing, like calculating some fields or deleting fileds).
  2. DTO's are automatically interfered by the Swagger plugin, so we don't need to annotate things with @ApiProperty() .
  3. We can make nestjs automatically strip away extra properties based on DTO's, so we're making sure that extra props are never sent
rajdip-b commented 4 months ago

One general suggestion that I would like you to have is, since we are using prisma, using a DTO for returning data doesn't really make much sense and seems like extra code.

It does make sense to use. DTO because:

1. Sometimes prisma types are different of what we actually want to send to the frontend (and sometimes include extra post processing, like calculating some fields or deleting fileds).

2. DTO's are automatically interfered by the Swagger plugin, so we don't need to annotate things with `@ApiProperty()` .

3. We can make nestjs automatically strip away extra properties based on DTO's, so we're making sure that extra props are never sent

Okay! Cool! Would you think it would be good idea to seperate the request and response dtos in two separate folders under the dto folder?

adelinaenache commented 4 months ago

One general suggestion that I would like you to have is, since we are using prisma, using a DTO for returning data doesn't really make much sense and seems like extra code.

It does make sense to use. DTO because:

1. Sometimes prisma types are different of what we actually want to send to the frontend (and sometimes include extra post processing, like calculating some fields or deleting fileds).

2. DTO's are automatically interfered by the Swagger plugin, so we don't need to annotate things with `@ApiProperty()` .

3. We can make nestjs automatically strip away extra properties based on DTO's, so we're making sure that extra props are never sent

Okay! Cool! Would you think it would be good idea to seperate the request and response dtos in two separate folders under the dto folder?

No, not necessarily. I feel like that would just create more overhead in the development since we usually have just one output DTO (that is why I implemented that transform... method in services, and the file names are enough to know which dto are you looking at (ie update, create, etc)

rajdip-b commented 4 months ago

we usually have just one output DTO (that is why I implemented that transform... method in services

The point im trying to make here is, say the number of endpoints grow. For the simplest of example, lets take the example of retrieving a list of items, and a specific item. For a specific item, you would want to have much more detailed data, and sometimes even nested data. For lists, you wont bother sending the entire thing, but just partial information that is needed by the UI. So if i call this entity as Item, we can have two DTOs - ItemDTO and ItemListDTO. The names are self-explanatory. In this case, we can drop the transform function and solely rely on the class types. You can refer to this. I feel it will be much more cleaner that way.

No, not necessarily. I feel like that would just create more overhead in the development

When we are trying to create resources, we will be sending in details like name, description, etc etc. We wont be having any ID or any nested mappings (most of the time). In this case, if we are relying on the same DTO for both creating and fetching our records, we will land up in trouble where the swagger parser will misunderstand what exactly it is that we need. Also, it won't make sense to use class validator annotations in fields of a dto class that is responsible for returning data. EDIT: I used the single dto approach in an enterprise and messed up the code real bad due to this exact same problem.