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

Improve each template literal infer #14920

Merged

Conversation

MathieuFedrigo
Copy link
Contributor

Summary

Small improvement to continue the work done mainly on this PR to improve type safety/inference on the Template literals syntax of .each.

Currently, when using the template literals syntax with .each, the args type is inferred if you don't provide the generic type argument.

If all inputs (variables inside the template literal) are of the same type, the args will be inferred to the same type.

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

But if the inputs are of different types, the args will be inferred to type unknown.

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}) => {
  // Without the generic argument explicitly provided, args types would default to `unknown`
});

This PR aims to improve the type inference for this second case. Now, args will be inferred to a type representing the union of all input types (automatically inferred from the input types inside the template literal).

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`
});

This provides more type safety IMO because otherwise, with type unknown you can cast to non-permitted types without any warning. It will also encourage the use of an explicitly provided generic which is the safest solution regarding types currently.

Test plan

The tests already existed but were expecting unknown. I updated them.

Copy link

linux-foundation-easycla bot commented Feb 23, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Feb 23, 2024

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit acf875e
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/65f6c09a1840130008615d16
😎 Deploy Preview https://deploy-preview-14920--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@MathieuFedrigo MathieuFedrigo force-pushed the improve-each-template-literal-infer branch from d030d04 to 38ca698 Compare February 23, 2024 18:08
Comment on lines 1093 to 1114
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'}
${2} | ${'two'}
${'will not raise TS error'} | ${'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 the the 3rd test case.
});
```
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This behaviour is not related to this PR, but since I'm editing the docs, I tried to clarify it 🙂

@@ -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?

@@ -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

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

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

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

@MathieuFedrigo MathieuFedrigo force-pushed the improve-each-template-literal-infer branch from 38ca698 to bdf450c Compare February 26, 2024 18:16
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

docs/GlobalAPI.md Outdated Show resolved Hide resolved
@SimenB SimenB merged commit ca363c8 into jestjs:main Mar 17, 2024
5 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024
@SimenB
Copy link
Member

SimenB commented May 12, 2024

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants