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.

Sign up for GitHub

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

Jump to bottom

Improve each template literal infer #14920

Merged
merged 6 commits into from
Mar 17, 2024
Merged

Improve each template literal infer #14920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9969aed
feat: improve type inference for template literals .each syntax
MathieuFedrigo Feb 23, 2024
f4f8533
docs: clarify type checking for template literals .each syntax
MathieuFedrigo Feb 23, 2024
bdf450c
add changelog entry
MathieuFedrigo Feb 23, 2024
b4af639
Merge branch 'main' into improve-each-template-literal-infer
SimenB Mar 17, 2024
fcc5d18
Update CHANGELOG.md
SimenB Mar 17, 2024
acf875e
Update docs/GlobalAPI.md
SimenB Mar 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
- `[jest-runtime]` Support `import.meta.resolve` ([#14930](https://github.com/jestjs/jest/pull/14930))
- `[@jest/schemas]` Upgrade `@sinclair/typebox` to v0.31 ([#14072](https://github.com/jestjs/jest/pull/14072))
- `[@jest/types]` `test.each()`: Accept a readonly (`as const`) table properly ([#14565](https://github.com/jestjs/jest/pull/14565))
- `[@jest/types]` Improve argument type inference passed to `test` and `describe` callback functions from `each` tables ([#14920](https://github.com/jestjs/jest/pull/14920))
- `[jest-snapshot]` [**BREAKING**] Add support for [Error causes](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause) in snapshots ([#13965](https://github.com/facebook/jest/pull/13965))
- `[jest-snapshot]` Support Prettier 3 ([#14566](https://github.com/facebook/jest/pull/14566))
- `[pretty-format]` [**BREAKING**] Do not render empty string children (`''`) in React plugin ([#14470](https://github.com/facebook/jest/pull/14470))
Expand Down
45 changes: 40 additions & 5 deletions docs/GlobalAPI.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1050,7 +1050,7 @@ test.each(table)('table as a variable example', (a, b, expected, extra) => {

#### Template literal

If all values are of the same type, the template literal API will type the arguments correctly:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea here was to start with a simple and correctly working example.

I find it strange showing how something does not work. If a feature does not work, why it is shipped? Just remove it.

It is just an option, but I think documentation should show how to use a library instead of focussing on what does not work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also your change makes this feature work better. Why to say something does not work? Hey, look! This works like a charm. Use it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what part you're referring to but I simplified it!

If case you were talking about the last part (2nd commit), I chose to leave it here, but more as a pitfall. wdyt?

If all input values are of the same type, the template literal API will type the arguments correctly:

```ts
import {test} from '@jest/globals';
Expand All @@ -1060,12 +1060,27 @@ test.each`
${1} | ${2} | ${3}
${3} | ${4} | ${7}
${5} | ${6} | ${11}
`('template literal example', ({a, b, expected}) => {
// all arguments are of type `number`
`('template literal example same type', ({a, b, expected}) => {
// all arguments are of type `number` because all inputs (a, b, expected) are of type `number`
});
```

Otherwise it will require a generic type argument:
If the inputs have different types, the arguments will be typed as a union of all the input types (i.e. type of the variables inside the template literal):

```ts
import {test} from '@jest/globals';

test.each`
a | b | expected
${1} | ${2} | ${'three'}
${3} | ${4} | ${'seven'}
${5} | ${6} | ${'eleven'}
`('template literal example different types', ({a, b, expected}) => {
// all arguments are of type `number | string` because some inputs (a, b) are of type `number` and some others (expected) are of type `string`
});
```

Otherwise, if you want each argument to have the right type, you have to explicitly provide the generic type argument:

```ts
import {test} from '@jest/globals';
Expand All @@ -1076,6 +1091,26 @@ test.each<{a: number; b: number; expected: string; extra?: boolean}>`
${3} | ${4} | ${'seven'} | ${false}
${5} | ${6} | ${'eleven'}
`('template literal example', ({a, b, expected, extra}) => {
// without the generic argument in this case types would default to `unknown`
// all arguments are typed as expected, e.g. `a: number`, `expected: string`, `extra: boolean | undefined`
});
```

:::caution

Keep in mind the variables inside the template literal are not type checked, so you have to ensure that their types are correct.

```ts
import {test} from '@jest/globals';

test.each<{a: number; expected: string}>`
a | expected
${1} | ${'one'}
${'will not raise TS error'} | ${'two'}
${3} | ${'three'}
`('template literal with wrongly typed input', ({a, expected}) => {
// all arguments are typed as stated in the generic: `a: number`, `expected: string`
// WARNING: `a` is of type `number` but will be a string in the 2nd test case.
});
```

:::
16 changes: 8 additions & 8 deletions packages/jest-types/__typetests__/each.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,8 +217,8 @@ expectType<void>(
${'a'} | ${true}
${'b'} | ${false}
`('some test', ({item, expected}) => {
expectType<unknown>(item);
expectType<unknown>(expected);
expectType<string | boolean>(item);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm.. to be honest I find this behaviour confusing. item cannot be boolean. Again, just an opinion, but isn't it better to explicitly tell that type cannot be inferred instead of providing misleading / incorrect type?

Copy link
Contributor Author

@MathieuFedrigo MathieuFedrigo Feb 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see what you mean. Not sure if there is a standard & this might ultimately be opinion.

To clarify my reasoning, there are 2 advantages to using a union imo:

  1. Less need to cast. When 1 input can actually be of different types (this might not happen often I recon, except maybe for generic functions):
test.each`
  a       | expected
  ${0}    | ${'0'}
  ${true} | ${'true'}
  ${'a'}  | ${'a'}
`('strange stringify', ({a, expected}) => {
  expect(toString(a)).toBe(expected); // no need to cast `a` here with union
});
  1. Identify abusive casting: you can cast unknown to anything, but not the union.
    (the following example is strange, but something similar could happen during refactoring or when a test is updated)
test.each`
  a    | b    | expected
  ${1} | ${2} | ${'three'}
  ${3} | ${4} | ${'seven'}
`('strange stringify', ({a, expected}) => {
  const shouldError = expected as any[]
  // With `unknown`: no error 😬
  // With union: Conversion of type 'string | number' to type 'any[]' may be a mistake because neither type sufficiently overlaps with the other.
});

wdyt?

Copy link
Contributor

@mrazauskas mrazauskas Feb 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposal, but I don’t see it as improvement. Wrong types are wrong types (;

This is only an opinion. I cannot merge PRs in this repo.

Copy link
Member

@SimenB SimenB Mar 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's an improvement to say "this can be a union of these types" over unknown, even if the union is too broad. Ideally we'd be able to key to the correct value, but I think this is an improvement, even if we'd want to go further

expectType<string | boolean>(expected);
}),
);
expectType<void>(
Expand Down Expand Up @@ -257,8 +257,8 @@ expectType<void>(
`(
'some test',
({item, expected}) => {
expectType<unknown>(item);
expectType<unknown>(expected);
expectType<string | boolean>(item);
expectType<string | boolean>(expected);
},
1000,
),
Expand Down Expand Up @@ -393,8 +393,8 @@ expectType<void>(
${'a'} | ${true}
${'b'} | ${false}
`('some test', async ({item, expected}) => {
expectType<unknown>(item);
expectType<unknown>(expected);
expectType<string | boolean>(item);
expectType<string | boolean>(expected);
}),
);
expectType<void>(
Expand Down Expand Up @@ -432,8 +432,8 @@ expectType<void>(
`(
'some test',
({item, expected}) => {
expectType<unknown>(item);
expectType<unknown>(expected);
expectType<string | boolean>(item);
expectType<string | boolean>(expected);
},
1000,
),
Expand Down
6 changes: 3 additions & 3 deletions packages/jest-types/src/Global.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,12 @@ interface Each<EachFn extends TestFn | BlockFn> {
) => void;

// when the table is a template literal
<T = unknown>(
<T extends Array<unknown>>(
strings: TemplateStringsArray,
...expressions: Array<T>
...expressions: T
): (
name: string | NameLike,
fn: (arg: Record<string, T>, done: DoneFn) => ReturnType<EachFn>,
fn: (arg: Record<string, T[number]>, done: DoneFn) => ReturnType<EachFn>,
timeout?: number,
) => void;

Expand Down
Loading