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

binded arguments in the "path" between function's decorator and parameters' decorators are different #896

Open
yadav-saurabh opened this issue May 4, 2024 · 11 comments
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested

Comments

@yadav-saurabh
Copy link

yadav-saurabh commented May 4, 2024

Having isuue while running npx nestia setup

Seems like Param's definition as an object is not supported, In Nestjs @Param() params: { id: string; countryId: string } is valid.
@TypedParam() doesn't support it yet.

@Get('/:countryId/states/:id')
  findOneState(@Param() params: { id: string; countryId: string }) {
    return this.countriesService.findOneState(+params.countryId, params.id);
  }
  - binded arguments in the "path" between function's decorator and parameters' decorators are different (function: [countryId, id], parameters: []).
@samchon samchon self-assigned this May 4, 2024
@samchon samchon added invalid This doesn't seem right help wanted Extra attention is needed question Further information is requested and removed invalid This doesn't seem right labels May 4, 2024
@samchon
Copy link
Owner

samchon commented May 4, 2024

@micalevisk Is this intended spec of NestJS? Or just unexpected behavior but just supported by express?

@yadav-saurabh If you want to generate SDK library through nestia, I think it would better to follow its guide. The params type is called object literal expression type (unnamed, implicit), and nestia does not allow such unnamed type. Also, considering the characteristics of @nestia/sdk, such spec can't be allowed because the client side SDK library should keep the consistency, but such way makes client developers confused.

export class SomeClass {
  public someMethod(
    @Param("a") a: string,
    @Param() params: {
      b: string,
      c: string,
    } {}
}

@micalevisk
Copy link

I don't see anything wrong with that code snippet

@samchon samchon added invalid This doesn't seem right wontfix This will not be worked on labels May 4, 2024
@samchon
Copy link
Owner

samchon commented May 4, 2024

image

Ah, understood. It was the original spec of NestJS. Thansk for quick response @micalevisk

In the object type case, how @nestjs/swagger composes the parameters (OpenApiOperation.parameters) ?

@samchon
Copy link
Owner

samchon commented May 4, 2024

@yadav-saurabh Is it okay to support object typed @Param(), but SDK library decomposes it to individual parameters?

If @nestjs/swagger supports the object type @Param() case with exact strategy, I'll support it.

@samchon samchon added enhancement New feature or request good first issue Good for newcomers and removed invalid This doesn't seem right wontfix This will not be worked on labels May 4, 2024
@yadav-saurabh
Copy link
Author

@yadav-saurabh Is it okay to support object typed @Param(), but SDK library decomposes it to individual parameters?

yes, my initial thinking was the same. They are going be individual parameters from the frontend POV, just nestjs (maybe express) makes an object so that's easier to access if there are multiple parameters.

@micalevisk
Copy link

In the object type case, how @nestjs/swagger composes the parameters (OpenApiOperation.parameters) ?

type defs like that has not effect for @nestjs/swagger (the CLI plugin)

@samchon
Copy link
Owner

samchon commented May 4, 2024

In the object type case, how @nestjs/swagger composes the parameters (OpenApiOperation.parameters) ?

type defs like that has not effect for @nestjs/swagger (the CLI plugin)

Then should I define @ApiParam() to the object typed path parameter, and write the object path parameter type with @ApiProperty()?

@micalevisk
Copy link

micalevisk commented May 4, 2024

it will work if you don't use types but concrete classes (with no decorators, if you're using the @nestjs/swagger plugin), due to how tsc works

or if you use @ApiParam(), of course

not sure about the behavior when using both approaches

@samchon
Copy link
Owner

samchon commented May 4, 2024

@micalevisk Thaks for help.

@yadav-saurabh To accomplish this mission, I've to disassemble the object typed @Param() in the compilation level, and it needs lots of change in both @nestia/core and @nestia/sdk projects. Furthermore, as @Param() is not of my development, but of pure NestJS feature, it may needs lots of time.

By the way, this mission's benefit for users is a little bit lower, so that would be delayed due to lower priority.

Until that, recommend to avoid using the object typed @Param().

@yadav-saurabh
Copy link
Author

@micalevisk Thaks for help.

@yadav-saurabh To accomplish this mission, I've to disassemble the object typed @Param() in the compilation level, and it needs lots of change in both @nestia/core and @nestia/sdk projects. Furthermore, as @Param() is not of my development, but of pure NestJS feature, it may needs lots of time.

By the way, this mission's benefit for users is a little bit lower, so that would be delayed due to lower priority.

Until that, recommend to avoid using the object typed @Param().

Yes, I can understand.

Thanks for the quick response

@samchon
Copy link
Owner

samchon commented May 4, 2024

Will start this ticket after #211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants