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

Replace functions returning explicit undefined value #77

Open
gildub opened this issue Jun 26, 2024 · 4 comments
Open

Replace functions returning explicit undefined value #77

gildub opened this issue Jun 26, 2024 · 4 comments

Comments

@gildub
Copy link
Contributor

gildub commented Jun 26, 2024

Undefined means no value has been assigned where null means an empty value.
It's a good practice to return and propagate a null value instead of using undefined because it requires to handle the special type case of undefined in the caller.

@helio-frota
Copy link
Collaborator

I would add this practice as well:

before

if (num === undefined) {
  return "";
}
return String(num);

after

if (!num) {
  return "";
}
return String(num);

or more fancy

export const numStr = (num: number | undefined): string => String(num ?? "");

console.log(numStr(1));
console.log(numStr(0));
console.log(numStr(undefined));

ts-playground

@carlosthe19916
Copy link
Member

I am not a fan of generic rules applied without reasoning. If there is a specific block of code where we consider we can achieve better readability, usability, performance, etc. then let's do it; let's open a PR and we can discuss what we are solving with that code change.

@helio-frota
Copy link
Collaborator

I think this is related with @gildub issue description to avoid runtime error reactwg/react-18#75
the idea is to delegate the check to the lint as trustify-ui is using react 18.

@helio-frota
Copy link
Collaborator

helio-frota commented Jun 26, 2024

For this we would need to enable the lint https://github.com/trustification/trustify-ui/blob/main/.github/workflows/ci-actions.yaml#L29
and add the suggested rules on https://github.com/trustification/trustify-ui/blob/main/.eslintrc.cjs#L40

    "consistent-return": "error", ( or warn ?)
    "no-unused-expressions": "error", ( or warn ?)

I have no idea if there are other rules for this, react-related

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

3 participants