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

Remove functions from enum values #2099

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

SimonRosenau
Copy link

@SimonRosenau SimonRosenau commented May 20, 2023

Description

This change removes function declarations from enum values.

I am using an enum with an companion namespace with utility functions:

export enum ResponseTypeEnum {
  Code = 'code',
}

export namespace ResponseTypeEnum {
  export const fromString = (value: string): ResponseTypeEnum => {
    switch (value.toLowerCase()) {
      case 'code':
        return ResponseTypeEnum.Code
      default:
        throw Error('Invalid response type: ' + value)
    }
  }
}

Within a request body as such:

export class Request {
  @IsEnum(ResponseTypeEnum)
  responseType: ResponseTypeEnum
}

The error returned by the server upon sending an invalid enum value is serialized as the following:

"responseType must be one of the following values: code, (value) => {\n        switch (value.toLowerCase()) {\n            case 'code':\n                return ResponseTypeEnum.Code;\n            default:\n                throw Error('Invalid response type: ' + value);\n        }\n    }"

This PR removes the function declaration from the possible enum values in the error message

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

@Clashsoft
Copy link
Contributor

Why not combine the filter operations to save an intermediate array creation?

@SimonRosenau
Copy link
Author

@Clashsoft In this case I would prefer code style in favor of performance, as honestly the performance gain would be quite minuscule, but if others would agree that this should be refactored I would also be open to do so

@Clashsoft
Copy link
Contributor

In addition I would even argue that the parseInt should be guarded with typeof value === 'string' instead of !== function because that is the assumption of as string in the last line. So some change is necessary anyway

@SimonRosenau
Copy link
Author

Hey @Clashsoft! Sorry for the long wait. I have addressed your requested changes, but now I'm not even sure whether the isNaN(parseInt(key)) check is even needed anymore?

@Clashsoft
Copy link
Contributor

Clashsoft commented Mar 11, 2024

@SimonRosenau Thanks for addressing! I believe you don't need it anymore. See https://stackoverflow.com/a/43091709/4138801

Object.keys(myEnum).map(key => myEnum[key]).filter(value => typeof value === 'string') as string[];

Or with ES2017 according to this comment:

Object.values(myEnum).filter(value => typeof value === 'string') as string[];

@SimonRosenau
Copy link
Author

@Clashsoft Makes absolute sense! Adjusted 🙂

@Clashsoft
Copy link
Contributor

LGTM!

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

Successfully merging this pull request may close these issues.

None yet

2 participants