[Luxon] Provide generics to allow individual types to be valid/invalid #65078
Conversation
|
@CarsonF Thank you for submitting this PR! This is a live comment which I will keep updated. 1 package in this PR
Code ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 65078,
"author": "CarsonF",
"headCommitOid": "f6274913355daaecd4c082d222e6d00c0de6ad14",
"mergeBaseOid": "0dad2b5fe7a3d49263d0b6ce12d2e892df1af9cc",
"lastPushDate": "2023-04-11T13:55:37.000Z",
"lastActivityDate": "2023-11-30T20:24:55.000Z",
"maintainerBlessed": "Waiting for Code Reviews",
"mergeOfferDate": "2023-11-30T19:56:42.000Z",
"mergeRequestDate": "2023-11-30T20:24:55.000Z",
"mergeRequestUser": "CarsonF",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "luxon",
"kind": "edit",
"files": [
{
"path": "types/luxon/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/luxon/src/_util.d.ts",
"kind": "definition"
},
{
"path": "types/luxon/src/datetime.d.ts",
"kind": "definition"
},
{
"path": "types/luxon/src/duration.d.ts",
"kind": "definition"
},
{
"path": "types/luxon/src/interval.d.ts",
"kind": "definition"
},
{
"path": "types/luxon/src/settings.d.ts",
"kind": "definition"
},
{
"path": "types/luxon/src/zone.d.ts",
"kind": "definition"
},
{
"path": "types/luxon/test/luxon-tests.module.ts",
"kind": "test"
}
],
"owners": [
"FourwingsY",
"jsiebern",
"mastermatt",
"pietrovismara",
"dawnmist",
"ycmjason",
"Aitor1995",
"peterblazejewicz",
"carsonf",
"hugofpsilva",
"martin-badin"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "peterblazejewicz",
"date": "2023-11-30T19:55:57.000Z",
"isMaintainer": true
},
{
"type": "stale",
"reviewer": "koooge",
"date": "2023-04-22T18:56:19.000Z",
"abbrOid": "597f3ba"
}
],
"mainBotCommentID": 1503434831,
"ciResult": "pass"
} |
typescript-bot
moved this from Needs Author Action
to Needs Maintainer Review
in New Pull Request Status Board
|
🔔 @FourwingsY @jsiebern @mastermatt @pietrovismara @dawnmist @ycmjason @Aitor1995 @peterblazejewicz @hugofpsilva @martin-badin — please review this PR in the next few days. Be sure to explicitly select |
|
does |
All of the |
|
Possibly a solution to the const dt = DateTime.fromISO('') as any as DateTime<true> | DateTime<false>;
dt.toISO() // string | null
if (dt.isValid) {
dt.toISO() // string
}Since Idk if it would make sense to change the names to something like: class DateTimeClass<...> {}
type DateTime = DateTimeClass<true> | DateTimeClass<false>;That probably wouldn't work. Maybe something like type DateTimeValidityUnion = ValidDateTime | InvalidDateTime;
type ValidDateTime = DateTime<true>;
type InvalidDateTime = DateTime<false>;Then all the factory methods would return 1 of those 3 types. |
RyanCavanaugh
moved this from Needs Maintainer Review
to Waiting for Code Reviews
in New Pull Request Status Board
|
Re-ping @horiuchi, @WinUP, @koooge, @FourwingsY, @jsiebern, @mastermatt, @pietrovismara, @dawnmist, @ycmjason, @Aitor1995, @peterblazejewicz, @hugofpsilva, @martin-badin: This PR has been out for over a week, yet I haven't seen any reviews. Could someone please give it some attention? Thanks! |
typescript-bot
added
the
Unreviewed
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Action
in New Pull Request Status Board
|
It has been more than two weeks and this PR still has no reviews. I'll bump it to the DT maintainer queue. Thank you for your patience, @CarsonF. (Ping @FourwingsY, @jsiebern, @mastermatt, @pietrovismara, @dawnmist, @ycmjason, @Aitor1995, @peterblazejewicz, @hugofpsilva, @martin-badin.) |
typescript-bot
moved this from Needs Maintainer Action
to Needs Author Action
in New Pull Request Status Board
|
It would be great to see some momentum on this PR. @CarsonF is there more work needed here? |
|
Honestly, I love the idea behind the #64995 change as it highlights a lurking issue via type checking, always a good thing. But with that must come a clean way to assert validity per standard type guards, etc. or you wind up peppering your code with |
CarsonF
force-pushed
the
luxon/valid-types
branch
2 times, most recently
from
9fb9e16
to
46a7a39
Compare
|
Just got the branch rebased with all the formatting changes that were conflicting. @pvogel1967 I think mainly the concern is just around just being able to drop this change in and it be the same. i.e. I haven't added required generics that need to be added everywhere. So type usage doesn't need to change at all in your repos. |
|
Yea I agree with @pvogel1967 and our codebase has also suffered from #64995 :) |
CarsonF
force-pushed
the
luxon/valid-types
branch
from
6272ca4
to
b20e0e1
Compare
|
I went with type Valid = true;
type Invalid = false;
Duration<Valid>
Duration<Invalid>So we don't have two more type aliases per class. |
typescript-bot
added
Maintainer Approved
Self Merge
typescript-bot
moved this from Needs Maintainer Action
to Waiting for Author to Merge
in New Pull Request Status Board
|
Ready to merge |
typescript-bot
moved this from Waiting for Author to Merge
to Recently Merged
in New Pull Request Status Board
We found (after being surprised by the typing issue - which I definitely agree is a positive thing) actually both usage patterns in our code base. In some parts, we used Please share your insights on moment/luxon#1421 to discuss how a (type)safe |
|
|
||
| /** | ||
| * Return an Interval spanning between this DateTime and another DateTime | ||
| * | ||
| * @param otherDateTime - the other end point of the Interval | ||
| */ | ||
| until(otherDateTime: DateTime): Interval; | ||
| until(otherDateTime: DateTime): IfValid<Interval<Valid>, DateTime<Invalid>, IsValid>; |
There was a problem hiding this comment.
I think this should have been
| until(otherDateTime: DateTime): IfValid<Interval<Valid>, DateTime<Invalid>, IsValid>; | |
| until(otherDateTime: DateTime): IfValid<Interval<Valid>, Interval<Invalid>, IsValid>; |
There was a problem hiding this comment.
https://github.com/moment/luxon/blob/master/src%2Fdatetime.js#L2081
Probably an oversight in JS src, but I felt like I should be true to it.
There was a problem hiding this comment.
Oh wow. Reported as moment/luxon#1551!
|
This is a breaking change in a minor version which causes some issues for us. Do you have any docs on how to migrate from version 3.3.5 to 3.3.6? |
|
@philipmagnusson how is it breaking? I left a couple more comments here, but generally I wouldn't expect any breaking changes here. |
|
You've changed the type of DateTime and the return types of most methods, such as fromISO (see attached images but I guess you know about these changes). fromISO 3.3.5: To me it's likely to be a breaking change, at least it's risky to do in a patch updates. Below are some of the outputs that we get from the typescript compile command after we updated from 3.3.5 to 3.3.6: In order to fix the compilation errors I have to do this in the code for intervals forexample: Here is another example that was introduced in 3.4.0: And this is how I worked around the issue:
|
|
@philipmagnusson we don't have a lot of control on versioning with breaking changes. We follow the src packages versioning, so really we can only do patch releases ourselves. It's a bit hard to know what the problem you are having is without being able to get my hands on the code. const breakInterval: Ref<Interval> = ref(Interval.fromISO())to work fine. I still recommend enabling luxon to throw on invalid input. Which does away with all of this validity generics. import {Settings} from 'luxon';
Settings.throwOnInvalid = true;
declare module 'luxon' {
interface TSSettings {
throwOnInvalid: true;
}
} |
|
Unfortunately, where
I'm not sure what it would been for our code base to opt-in to |
|
@philipmagnusson I'm assuming you are assuming all temporal inputs are valid, and not checking the Vue's |
|
hmm... I don't understand. Should the settings you posted above remove the DateTime<true|false> definitions? I tried it but I don't think it works, or I don't understand how to insert the settings you posted above: Tried to put it in create a and do the setting: in the The DateTime is still defined as:
and e.g. setLocale returns |
|
+1 that this is breaking change: |
|
@philipmagnusson sorry I missed your reply. The signature you posted class DateTime<IsValid extends boolean = true>Means that the How is " Would you open a new discussion or issue to continue this? @trevorr "+1" comments are a frowned upon practice. It causes notification spam to maintainers and everyone participating in the thread without adding any value. Please don't do that anywhere. This is exactly what reactions are for. Both comments & reactions are easily ignored on merged PRs. That error doesn't tell me anything without seeing the source code that produced it. |
@CarsonF That's a bit condescending. It wasn't just a +1, since I included an additional error mode not yet mentioned. The error itself makes clear that the new type argument and its new usage within the interface are inherently breaking. Here's the test that now fails to type check: const startDate = DateTime.fromISO("2020-02-29T08:00:00Z", { zone: "utc" });
const pt2h = Duration.fromObject({ hours: 2 });
describe("formatRecurrenceRule", () => {
it("formats none", () => {
// error TS2345: Argument of type 'DateTime<true> | DateTime<false>' is not assignable to parameter of type 'DateTime<true> | undefined'.
expect(formatRecurrenceRule({ type: RecurrenceType.None }, startDate, pt2h)).toBeNull();
});Here's the signature of that function: export function formatRecurrenceRule(
recur: Recurrence,
startDate = DateTime.utc(),
duration?: Duration
): string | null { |
I understand typescript's error message. I guess what I meant was I still needed more info about the usage. So here's the rub. DateTime.utc() // always valid
DateTime.fromISO('...') // maybe valid depending on stringNow your export function formatRecurrenceRule(
recur: Recurrence,
startDate: DateTime = DateTime.utc(),
duration?: Duration
): string | null {This is the same principle as // Loosen name to any string, not just "Carson"
const name: string = "Carson";This is an unfortunate consequence of this PR, but I don't see any way around it. |
|
Yeah, that totally makes sense and thanks for the explanation. That change does fix all the compile errors in my tests. Perhaps this is mainly an issue when using type inference. Maybe what's needed is some sort of migration guide? It took a bit of digging to find this PR and figure out what was going on. Has anyone asked if the Luxon maintainers would accept some Typescript documentation and possibly even the typings into their repo? It seems like you (@CarsonF) did that in this PR for how to configure the default behavior. Sadly, you only got some not helpful feedback from a non-maintainer. |
|
I don't know what Issac's views on TS are. I've never seen him anywhere near these type PRs/issues (here or in his own repo). So yeah at the end of the day this is not a TS library, and TS is a second class citizen for it. So that excludes docs, breaking changes, etc related to types. |
Following up on #64995
In certain situations it's possible to know statically that a
DateTime(etc) is valid, even without thethrowOnInvalidenabled.For example, it's probably common to do
This will always return a string, as now() always returns a valid DateTime.
This PR enables TS to know that.