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

Better signature for optional function parameters #204

Open
scambier opened this issue Mar 18, 2022 · 2 comments
Open

Better signature for optional function parameters #204

scambier opened this issue Mar 18, 2022 · 2 comments
Assignees

Comments

@scambier
Copy link

Hello, I'm just trying rot.js, and I noticed that several functions have optional parameters that are typed as SomeType | null (like here).

The issue is that TypeScript does not consider those parameters as optional: you either give a value, or an explicit null:
image

A more correct function signature would be

draw(x: number, y: number, ch?: string | string[], fg?: string, bg?: string) {
/** **/
}

I guess it should be an easy fix (look for all | null in the code), but as I'm literally discovering the lib, I'm not too confident on making a PR for now.

@ondras
Copy link
Owner

ondras commented Mar 23, 2022

Hi @scambier,

yeah, you are completely correct. I am really not sure why we have these weird null-based signatures - perhaps to comply with existing (pre-TS) code? Passing null to an optional argument would work fine though, provided we test those values with simple if (!fg) ....

@ondras ondras self-assigned this Mar 23, 2022
@scambier
Copy link
Author

Passing null to an optional argument would work fine though, provided we test those values with simple if (!fg) ....

Sure, and you'll have to do the nullish check anyway because the default value for optional arguments is undefined. Calling draw(5, 4, "@") would be the same as calling draw(5, 4, "@", undefined, undefined).

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

No branches or pull requests

2 participants