nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.73k stars 7.63k forks source link

Update JSDoc for Exception classes with a number type as the first parameter. #12787

Closed coldMater closed 21 hours ago

coldMater commented 11 months ago

Did you read the migration guide?

Is there an existing issue that is already proposing this?

Potential Commit/PR that introduced the regression

https://github.com/nestjs/nest/pull/10030/files

NestJS version

9.1.2 -> 10.2.6

Describe the regression

Previously, a numeric value was passed to UnauthorizedException (and other exceptions), and the createBody() function returned an object like {statusCode: 401, message: 40108, error: "Unauthorized"}.

But now, when the createBody() function receives a numeric value as the constructor's first parameter, it returns that argument as is, without converting it to an object.

We manage that object as the response property of the exception instance in my custom Exception Filter, and some unexpected behavior has occurred. (When exceptionResponse, which was supposed to be of object type, is passed as a number type, errorCode becomes NaN, causing issues in the subsequent logic.)

// part of business logic

const ErrorCode = {
  INVALID_PASSWORD: 40123
}

throw new UnauthorizedException(ErrorCode.INVALID_PASSWORD)
// part of custom.exception.filter.ts

async catch(exception: HttpException, host: ArgumentsHost): Promise<any> {
  // Omitted some code in the middle
  const exceptionResponse = exception.getResponse() // Once it was an object, but now it is just a number.
  const errorCode = Number(exceptionResponse.message) // 
  // ...

Minimum reproduction code

No response (Please Refer to the above code)

Input code

No response (Please refer to the above code)

// part of custom.exception.filter.ts

async catch(exception: HttpException, host: ArgumentsHost): Promise<any> {
  // Omitted some code in the middle
  const exceptionResponse = exception.getResponse() // once object 
  const errorCode = Number(exceptionResponse.message) // 
  // ...

Expected behavior

Suggestion 1. Update the JsDoc for the Exception-Like class

To the best of my knowledge, I couldn't find any relevant information about this in recent migration or release documents. (If I overlooked any relevant information, I sincerely apologize in advance for not finding it.) However, I believe that having such information in the JsDoc would have helped me identify the error I encountered more quickly. https://github.com/nestjs/nest/blob/8bba897216a1629ffc8e7029645f99fc9644a938/packages/common/exceptions/unauthorized.exception.ts#L28-L31

Suggestion 2. Remove the any type from the Exception constructor parameter

I wonder why the call signature of HttpException differs from its child class UnauthorizedException. If NestJS intends it to be specified as either a string or an object(not a number), I believe it should be explicitly asserted. I believe that the constructor function for UnauthorizedException should have the following type. I am also curious if there is a consolidated and reconciled opinion within the NestJS community regarding whether it can be of type any.

  constructor(
    objectOrError?: string | object, // `any` removed
    descriptionOrOptions: string | HttpExceptionOptions = 'Unauthorized',
  ) {

Suggestion 3. Ensure the createBody function behaves as it did before for number types

Following the handling in #10030 , I believe that the behavior of the createBody() function when receiving a numeric type as the sole argument should remain consistent with the previous implementation (version 9). However, if the current behavior in version 10 has been consolidated and reconciled within the NestJS community, I hope that Suggestion 1 or Suggestion 2 would be accepted by the community.

Other

No response

micalevisk commented 11 months ago

btw that was not a regression because of this:

image

but yeah, looks like we need to improve it

iannak commented 10 months ago

Hello, has it been resolved yet? Can you get it to resolve it?

I would like to contribute to the community!

micalevisk commented 10 months ago

@iannak PRs are more than welcomed

iannak commented 10 months ago

@iannakPRs são mais que bem-vindos

OK! I will analyze!

bipin0x01 commented 5 months ago

Any updates on this issue ?

iannak commented 5 months ago

Not yet, but by Monday I'll see if I can upload an update

mrxuanz commented 5 days ago

Hello! Does this issue need someone to work on it right now? I’d really like to contribute and do my part.

micalevisk commented 5 days ago

@mrxuanz PRs are more than welcomed. But I'd wait for Kamil's feedback first.

mrxuanz commented 5 days ago

@micalevisk Thank you very much.

kamilmysliwiec commented 21 hours ago

https://github.com/nestjs/nest/pull/14131