fix(assert): fail type check for assertArrayIncludes when element type of expected not assignable to actual (#6427)
#6428
Conversation
…ype of `expected` not assignable to `actual` (denoland#6427)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6428 +/- ##
==========================================
+ Coverage 96.03% 96.08% +0.04%
==========================================
Files 562 562
Lines 42418 42418
Branches 6387 6387
==========================================
+ Hits 40738 40756 +18
+ Misses 1641 1629 -12
+ Partials 39 33 -6 ☔ View full report in Codecov by Sentry. |
| @@ -18,7 +18,7 @@ | |||
| * | |||
| * assertEquals(intersect(lisaInterests, kimInterests), ["Cooking", "Music"]); | |||
| * | |||
| * assertArrayIncludes(lisaInterests, [sample(lisaInterests)]); | |||
| * assertArrayIncludes(lisaInterests, [sample(lisaInterests)!]); | |||
There was a problem hiding this comment.
hmm this change seems concerning to me (I think this PR will break actual users code in a similar way)
Can we keep this case working without change somehow?
There was a problem hiding this comment.
hmm this change seems concerning to me (I think this PR will break actual users code in a similar way)
Can we keep this case working without change somehow?
I don't think so, without explicitly allowing undefined, which seems wrong.
For comparison, again with Array#includes:
declare let xs: string[]
declare let x: string | undefined
// Argument of type 'string | undefined' is not assignable to parameter of type 'string'.
xs.includes(x)But on second thoughts, maybe the strict behavior rejecting legitimate uses is more annoying than the sloppy behavior accepting illegitimate ones/lacking autocomplete... it's not uncommon that I have to write code like this:
stringLiteralUnionArr.includes(arbitraryString as typeof stringLiteralUnionArr[number])
|
While I think this change is aligned to the direction we chose in #2203, I'm also very concerned with the breaking nature of this PR. cc @bartlomieju @ry |
|
I tend to agree. Maybe try using this PR in the Deno repo submodule and see how bad it's gonna be? |
|
It turned out that denoland/deno repo doesn't include any usage of There seem relatively small number of usages in github search https://github.com/search?q=assertArrayIncludes+language%3ATypeScript&type=code&l=TypeScript Maybe we can land this as 'fix'? |
Fixes #6427