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

unicorn/prefer-native-coercion-functions doesn't work well with typescript and should be disabled. #242

Open
hjeti opened this issue Mar 1, 2024 · 6 comments

Comments

@hjeti
Copy link

hjeti commented Mar 1, 2024

The Boolean part of this rule and the auto fix break my code, because they don't work well with typescript.

I write this:

const testArray = ['test', null];
        
const output = testArray.filter((value): value is string => Boolean(value));

and it converts it to this:

const testArray = ['test', null];

const output = testArray.filter(Boolean);

This is not the same. The type of output is Array<string> in my code and Array<string | null> in the 'autofixed' code.

@ThaNarie
Copy link
Member

ThaNarie commented Mar 1, 2024

It seems there is an open issue for this: sindresorhus/eslint-plugin-unicorn#1857

Alternative option is to use isBoolean from isntnt, which is typeguarded.

But it seems that, until that issue is fixed, we should disable it.

@leroykorterink
Copy link
Collaborator

Typescript now infers type predicates properly, but with this change I think we should disallow the use of the Boolean constructor as an argument for the Array.filter function.

https://devblogs.microsoft.com/typescript/announcing-typescript-5-5-beta/#inferred-type-predicates

@leroykorterink
Copy link
Collaborator

leroykorterink commented Feb 17, 2025

Typescript doesn't infer the types correctly for the Boolean constructor. The type guard is a type cast, it's unsafe and shouldn't be the preferred method to narrow the type.

  const test = [1, 2, 3, undefined];
  const hey: number[] = test.filter((item) => item !== undefined);
  const test = [1, 2, 3, undefined];
  const hey1: (number | undefined)[] = test.filter((item) => Boolean(item));
  const hey2: (number | undefined)[] = test.filter(Boolean);

@ThaNarie
Copy link
Member

ThaNarie commented Feb 17, 2025

So conclusion, we should still disable the rule until microsoft/TypeScript#16655 is fixed in TS.

What Leroy says below.

@leroykorterink
Copy link
Collaborator

Well, I think the conclusion is to stop using type guards when it's not necessary. Instead of relying on coercion to narrow a type it's better to implement a stricter condition. Disabling the rule doesn't discourage people from using type guards.

@ThaNarie
Copy link
Member

I've been playing around in the TS playground:
https://www.typescriptlang.org/play/?#code/PTAEFcGcEsDsHNQHdoBcAWpbgLYCMBTAJ0lGlIBMBDBYgeygCgBjO2SVUAD1AF5QA2gAYANKACMYgExiAzGOwAbRWPCwKBAGZwCFALqgqpVu1QBuRiFDXQAPQD8jFmw7dxfbgDpti1MQAUAEJ0dIoENACUFlY2do4mrlxSHlze0L4B0HwAfKAAhHnQUZZgsQ5OJRAwCMhomBxEcPCk5KDUtEQMkM6moACeHgIA5ENiQ1R4zBSjWODKqupaOvqGxi7mlTblPa597vx9aRlEQSFhkdGlW-Hr-ckHR34nWby5BUWXsXEVVlBNtRhQHQ8AArAjMVAtUjaWAEHacABegwA3lQAFwSAC+CjmKggixhugMRlACQ2MWs2zJoAR+xpjwCwVC4VgxQplJuvQR93pPie-heb0KbKuHMYQA

Boolean doesn't work, but !! does work (has equivalent runtime behaviour).
But as HJ pointed out, !! is not allowed, and is replaced by Boolean

Image

In general:

  • using Boolean/!! for numbers/strings can be dangerous (due to empty strings or 0 values that you might not want to filter out)
  • using Boolean/!! for objects is perfectly safe, so ideally we DO want to allow the !! use case (or later Boolean when TS is fixed).

From the original post
testArray.filter((value): value is string => Boolean(value)); should not be used (for strings)

So to conclude, using !! in filters for objects would be preferred, but unicorn/prefer-native-coercion-functions changes it to Boolean, which doesn't work in TS.
This is slightly different from the original post, there it's about typeguards. However, the typeguard Boolean to what it should do it if worked, similar to !!.

So in filters, I want to use !! in some cases (and not Boolean due to bugged behaviour).

However, in general I do prefer to use Boolean() over !! (since more readable/explicit).

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

3 participants