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

Consider limiting input type to string #142

Closed
harveysanders opened this issue Mar 20, 2024 · 4 comments · Fixed by #143
Closed

Consider limiting input type to string #142

harveysanders opened this issue Mar 20, 2024 · 4 comments · Fixed by #143

Comments

@harveysanders
Copy link
Contributor

Description

The type for parameter input in normalizeSync implies that undefined and null are acceptable inputs. However, the first line of the function throws an error if the input is not a string.

https://github.com/motss/normalize-diacritics/blob/main/src/normalize-sync.ts#L3-L6

I believe it would be more ergonomic to have TypeScript signal to the dev that only strings are acceptable.
In my opinion, TS is best suited to help devs avoid runtime errors by attempting to move common issues to compile-time errors. With this in mind, I think it would be more helpful to set input to only string.

export function normalizeSync(input: string): string {
  //....
}

That way we get type errors before running the code:

/** @type {string | undefined | null} */
const input = getInputFromWherever();

normalizeSync(input);
              ^^^^
              // Argument of type 'string | null | undefined' 
              // is not assignable to parameter of type 'string'.
              // Type 'undefined' is not assignable to type 'string'.ts(2345)

What do you think?

Anyway, thanks for building this lib 🙂

@harveysanders
Copy link
Contributor Author

If you agree, I'm happy to submit a PR : )

@motss
Copy link
Owner

motss commented Mar 21, 2024

@harveysanders Thanks for the feedback. I initially included null and undefined because of JavaScript being a dynamic language. I really like your explanation that TypeScript should tell the users the correct type and totally agree with your points.

Please help raise a PR to fix this.

@harveysanders
Copy link
Contributor Author

@motss thanks for the quick response. Sounds good! I'll try to get one in next week : )

@harveysanders
Copy link
Contributor Author

@motss I've just submitted a PR

motss added a commit that referenced this issue Mar 24, 2024
* fix(#142):  update normalizeSync input type to string only
* Update README.md

---------

Signed-off-by: Harvey Sanders <harveysanders@users.noreply.github.com>
Co-authored-by: The web walker <contact@motss.app>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants