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

Fix: Improve typing for min/max #2573

Merged
merged 3 commits into from
May 20, 2024
Merged

Conversation

bensaufley
Copy link
Contributor

I think it's good to be clear that null can be returned from min/max, but TypeScript is powerful enough to be even smarter about this. This PR updates the types so that at least obvious scenarios where at least one date is passed don't then require ! or other code smells.

As with all types as they get more complex, it's easy to miss things or forget scenarios. Please let me know if I've missed anything.

I wouldn't actually consider this a "fix" so much as an improvement on existing types but I don't think it's a feature and since the prior revision was marked as a fix, I'm keeping in line with that change.

No null if at least one date is passed; only null if explicitly no dates are passed.
@iamkun
Copy link
Owner

iamkun commented Apr 28, 2024

Any live example of this type update, please?

@bensaufley
Copy link
Contributor Author

bensaufley commented Apr 28, 2024

@iamkun Happy to provide examples; is there a place in the codebase I should do it?

For me, the issue the previous PR introduced is this (example code):

const myDate = dayjs('2023-08-17');
const anotherDate = dayjs(userInput);

if (!anotherDate.isValidDate()) return null;

return dayjs.min(myDate, anotherDate).add(1, 'week');

With the prior change, TS tells me .add isn't safe because the result of .min(…) may be null (and thus that I should put a ? before .add). But it will never be null; TypeScript reasonably guarantees that the two arguments passed in are both dayjs objects.

The function only takes Dayjs objects, but may technically be called with an empty array, in which case it will return null. But that's the only case in which it will return null. So if TS knows it's receiving at least one argument, it can also know that the min/max functions will return a Dayjs object. The rules are, or seem to be:

  • If min/max receives at least one Dayjs object, it will return a Dayjs object.
  • If it receives zero objects, it will return null.
  • It may be passed the array as separate args or as a single arg with an array inside it; this does not change the behavior

TypeScript doesn't always know whether an array has at least one value: Dayjs[] only says it's an array that can only contain Dayjs objects, but it can contain zero or more. In these cases, TS can't know whether or not min/max will return null or a Dayjs object and must assume either can be returned. So:

  • If args are ...Dayjs[] or Dayjs[]: return null | Dayjs
  • If args are an explicitly empty array (min(): ...never[] or min([]): never[]): return only null
  • If args are an array with at least one Dayjs element (min(foo, bar, baz): ...[Dayjs, ...Dayjs[]]) or min([foo, bar, baz]): [Dayjs, ...Dayjs[]]): return only Dayjs

The types are sorted by specificity (match "has Dayjs" first, match "explicitly empty" second, match "not sure" third) because TS will apply the first match it finds, so you don't want "not sure" at the top or it will always match.

Looking at the code now it looks like the function may also accept an explicit null (and return null). I can add typing for that, if it's valuable. That would be:

min(val: null): null; // explicitly null
min(vals: null | Dayjs[]): Dayjs | null // nullable but not definitely null
max(val: null): null;
max(vals: null | Dayjs[]): Dayjs | null // nullable but not definitely null

It does not look like min([null]) would be valid, as the check for truthiness happens before extracting args-as-array (so no min(val: null[])), and .isValid() is called on each element in the array, which would throw an error on null.

I know some codebases have "type tests" or similar but I couldn't find any here. If you would like something like that can you point me in their direction? I didn't see any in the original PR.

@bensaufley
Copy link
Contributor Author

@iamkun is there anything else I can tell you about this PR, anything else I should do?

@iamkun iamkun merged commit 4fbe94a into iamkun:dev May 20, 2024
4 checks passed
@iamkun
Copy link
Owner

iamkun commented May 20, 2024

excellent TypeScript example, Thanks.

@koengommers
Copy link

koengommers commented May 29, 2024

@iamkun Could this be released? Would help us out a ton.

@bensaufley bensaufley deleted the better-minmax-types branch May 29, 2024 16:16
github-actions bot pushed a commit that referenced this pull request Jul 18, 2024
## [1.11.12](v1.11.11...v1.11.12) (2024-07-18)

### Bug Fixes

* Add NegativeYear Plugin support  ([#2640](#2640)) ([6a42e0d](6a42e0d))
* add UTC support to negativeYear plugin ([#2692](#2692)) ([f3ef705](f3ef705))
* Fix zero offset issue when use tz with locale ([#2532](#2532)) ([d0e6738](d0e6738))
* Improve typing for min/max plugin ([#2573](#2573)) ([4fbe94a](4fbe94a))
* timezone plugin currect parse UTC tz ([#2693](#2693)) ([b575c81](b575c81))
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

Successfully merging this pull request may close these issues.

3 participants