luxon icon indicating copy to clipboard operation
luxon copied to clipboard

Type for validated DateTime

Open Razi91 opened this issue 1 year ago • 15 comments

Is your feature request related to a problem? Please describe. With @types/luxon in version 3.3.0 it checks if the DateTime may be invalid, forcing to check if the result isn't null

const dateInJson = DateTime
    .local() // DateTime
    .startOf('day') // DateTime
    .plus({months: 3}) // DateTime
    .toJSON() // string | null

Describe the solution you'd like A method like .validated() that either throw exception or return Validated<DateTime> skipping the possibility of null at .toJSON()

const dateInJson = DateTime
    .local() // DateTime
    .startOf('day') // DateTime
    .plus({months: 3}) // DateTime
    .validated() // Validated<DateTime> or throws exception
    .toJSON() // string

I'm not sure if .local shouldn't be already valid, startOf and plus shouldn't change it, therefore .validated shouldn't be even required here.

Razi91 avatar Apr 11 '23 10:04 Razi91

+1 to this - the compiler should be informed that DateTime.now().toISO() will always be string, because .now() should always be valid.

zackdotcomputer avatar Apr 11 '23 22:04 zackdotcomputer

Looks like a throwOnInvalid type settings has been introduced in version 3.3.0 (see PR). As documented on the PR, the type issue can be fixed by overriding the throwOnInvalid type setting like:

// luxon.d.ts
import 'luxon';

declare module 'luxon' {
    interface TSSettings {
        throwOnInvalid: true;
    }
}

And setup Luxon to throw error on invalid dates as documented here.

cgero-eth avatar Apr 12 '23 09:04 cgero-eth

I think the behavior @Razi91 has suggested would be an extension of throwOnInvalid - a way to ensure validity selectively rather than across the entire library. For example, there are some situations where you want to ensure validity absolutely (e.g. if loading a serialized date from a database) and some where you can tolerate failure (e.g. when accepting user input). As a workaround for now, I'll enable the throwOnInvalid behavior and use try/catch but being able to do this imperatively would feel more natural to me in TS.

As a side note, it would be great if the required snippets for enabling throwOnInvalid in both the code and typing were included in the documentation.

zackdotcomputer avatar Apr 12 '23 12:04 zackdotcomputer

I also stumbled upon this issue after recent updates to typings. However I would like to suggest a different solution to the luxon library itself.

Instead of a global setting that one must not forget to set in every importing module luxon could export a "strict" version of API that will have throwOnInvalid = true. That way developer can chose which version to import and it will always be correctly typed.

I think this is better since it doesn't pollute API with extra .validated() method that I would need to call on every construction of luxon dates. It also allows for finer choice of where to use strict mode and where not, unlike a global nature of static settings (can lead to subtle bugs when set from different places) and global type override.

shishkin avatar Apr 15 '23 07:04 shishkin

I like the idea @shishkin, since yes there is risk that the Settings value gets set or unset somewhere (potentially even within a dependency!) and introduces bugs. But I would suggest you move it to its own issue so it doesn't get missed by the maintaining team. I'd say it's solving a distinct enough problem from the one we're looking at that both should stand in their own right and the library would be better if the team was able to incorporate both, not just one or the other.

zackdotcomputer avatar Apr 15 '23 10:04 zackdotcomputer

@zackdotcomputer you're right. Just created #1421.

shishkin avatar Apr 15 '23 15:04 shishkin

Here for the following new type issue:

const test: string = DateTime.now().toISODate()
// Type 'string | null' is not assignable to type 'string'.⏎ Type 'null' is not assignable to type 'string'.

How exactly are we supposed to be handling this now? Is there some IsValid helper we can utilize? Documentation still shows that toISODate should always return a string.


Edit

After playing with this, I guess I need to handle nullability by adding a check and handling it, or defaulting it to some value.

const testOrNull: string | null = DateTime.now().toISODate()

if (testOrNull === null) {
  // do something; return or throw
}

const test: string = testOrNull // this will be valid now that we checked for the null scenario

For this it might be nice to utilize something like Results from https://github.com/vultix/ts-results so that you can denote success or failure explicitly, while having code that does not throw, and leaving it up to the consumer to handle the result type.

ctsstc avatar Apr 18 '23 21:04 ctsstc

Just ran into this tonight while trying to bump from v2.5 to v3.3 and have to say its very unexpected and is a major impact. We use Prisma and because of the introduction of this possible null result literally every place to convert a DateTime to an ISO string is now busted. Frustrating to say the least especially considering .now() should always be valid.

dvins avatar Jun 13 '23 01:06 dvins

I can't make any runtime changes, but I've improved the types: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/65078

DateTime.now().toISO(); // string
const input = DateTime.fromISO('...');

input.toISO(); // string | null

if (input.isValid) {
  input.toISO(); // string
  input.invalidReason; // null
} else {
  input.toISO(); // null
  input.invalidReason; // string
}

This is powered by a validity boolean generic. The default value is either valid or invalid (unless throwOnInvalid is enabled). To communicate a date was previously validated you can provide this value explicitly (true for valid, and false for invalid).

const now = DateTime.now();

function printDate(date: DateTime<true>): string {
  return date.toISO();
}
printDate(now);

CarsonF avatar Dec 06 '23 03:12 CarsonF

After playing with this, I guess I need to handle nullability by adding a check and handling it, or defaulting it to some value.

const testOrNull: string | null = DateTime.now().toISODate()

if (testOrNull === null) {
  // do something; return or throw
}

const test: string = testOrNull // this will be valid now that we checked for the null scenario

I found a much simpler way to deal with this breaking change for my cases was to use the nullish coalescing operator, like this:

const myString: string = myDate.toISODate() ?? '';

Andrew-Marks-Trisept avatar Dec 08 '23 21:12 Andrew-Marks-Trisept

@Andrew-Marks-Trisept that handles the typing, but likely you will not want to surface an empty string, most likely you'd want to handle it as an error or something rather than defaulting, in which case you'll end up with a typeguard/type narrowing instead.

ctsstc avatar Dec 12 '23 00:12 ctsstc

@CarsonF I think some of the type signatures are wrong in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/65078

At least for interval

     /**
     * Returns an error code if this Interval is invalid, or null if the Interval is valid
     */
-    get invalidReason(): string | null;
+    get invalidReason(): IfValid<string, null, IsValid>;

     /**
     * Returns an explanation of why this Interval became invalid, or null if the Interval is valid
     */
-    get invalidExplanation(): string | null;
+    get invalidExplanation(): IfValid<string | null, null, IsValid>;

I believe these should be: IfValid<null, string, IsValid>. Do you agree?

I haven't looked further beyond that, but just noticed this error pop up in my code when trying to narrow types:

if (interval.isValid) {
  // do stuff
} else {
  const error: string = interval.invalidExplanation; //  Type 'null' is not assignable to type 'string'.
}

BrentSouza avatar Jan 17 '24 17:01 BrentSouza

@BrentSouza Yes, you're right. My bad. Looks like DateTime was fixed in DefinitelyTyped/DefinitelyTyped#67756. Do you want to PR a fix?

CarsonF avatar Jan 17 '24 17:01 CarsonF

Sure. I'll open a PR shortly

BrentSouza avatar Jan 17 '24 18:01 BrentSouza