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

My favorite terrible place #19

Open
tshemsedinov opened this issue Mar 16, 2024 · 6 comments
Open

My favorite terrible place #19

tshemsedinov opened this issue Mar 16, 2024 · 6 comments

Comments

@tshemsedinov
Copy link

  1. Union type number | undefined is not ok here, why should we pass undefined here?
  2. Hardcoded http error codes without names, without named constants
  3. Expression code >= 100 && code < 600 in if statement returns boolean itself, so we do not need to use return true/return false
  4. Where can we use isHttpCode? Code range have gaps, so this will not validate, see RFC http for reference
    static isHttpCode(code: number | undefined): boolean {
    if (typeof code !== 'number') {
    return false
    }
    if (code >= 100 && code < 600) {
    return true
    }
    return false
    }
@tshemsedinov
Copy link
Author

Same problem here:

static isGrpcCode(code: number | undefined): boolean {
if (typeof code !== 'number') {
return false
}
return this.grpcStatusCodes.includes(code)
}

@vird
Copy link

vird commented Mar 17, 2024

Just as an idea, needs reworking to be more matched with code style
#26

@tshemsedinov
Copy link
Author

@vird Node.js have status codes collection: https://nodejs.org/api/http.html#httpstatus_codes

@vird
Copy link

vird commented Mar 17, 2024

I'm not sure we can use that here.
Didn't investigate if any other packages are trying to use this one at frontend

@tshemsedinov
Copy link
Author

be-* package names is backend @vird

@vird
Copy link

vird commented Mar 18, 2024

Agree
But with webpack any package which was previously named "for backend" can be used on frontend
We have no information about non-released parts of diia

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

No branches or pull requests

2 participants