[luxon] Changed typings for some datetime and duration functions to return string or null

[TomWeStartFires]

The luxon docs claim that certain function will return a string but instead they may return a string OR null (when the DateTime object is invalid). https://moment.github.io/luxon/docs/class/src/datetime.js~DateTime.html#instance-method-toISO

Examples include:

• luxon.DateTime.fromISO("").toISO()
• luxon.DateTime.fromISO("").toISODate()
• luxon.DateTime.fromISO("").toISOTime()
• luxon.DateTime.fromISO("").toISOWeekDate()
• luxon.DateTime.fromISO("").toJSON()
• luxon.DateTime.fromISO("").toLocaleString()
• luxon.DateTime.fromISO("").toRFC2822()
• luxon.DateTime.fromISO("").toSQL()
• luxon.DateTime.fromISO("").toSQLDate()
• luxon.DateTime.fromISO("").toSQLTime()
• luxon.Duration.fromISO("").toISO()
• luxon.Duration.fromISO("").toJSON()
• luxon.Duration.fromISO("").toString()

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: https://moment.github.io/luxon/docs/class/src/datetime.js~DateTime.html#instance-method-toISO (claims it will always return string, but actually the function can return null)
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
• Include tests for your changes
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

[typescript-bot]

@TomWeStartFires Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

• ✅ No merge conflicts
• ✅ Continuous integration tests have passed
• ✅ Most recent commit is approved by type definition owners or DT maintainers

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": 46290,
  "author": "TomWeStartFires",
  "owners": [
    "colbydehart",
    "FourwingsY",
    "jsiebern",
    "mastermatt",
    "pietrovismara",
    "dawnmist",
    "ycmjason",
    "Aitor1995",
    "peterblazejewicz"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "b5cce02",
  "headCommitOid": "b5cce02449ad93f60b4aee5ad75e387e42ccf23e",
  "mergeIsRequested": true,
  "stalenessInDays": 0,
  "lastPushDate": "2020-07-23T14:00:58.000Z",
  "lastCommentDate": "2020-07-23T19:41:18.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/46290/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": true,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "luxon"
  ],
  "files": [
    {
      "path": "types/luxon/index.d.ts",
      "kind": "definition",
      "package": "luxon"
    },
    {
      "path": "types/luxon/luxon-tests.ts",
      "kind": "test",
      "package": "luxon"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-07-23T19:24:48.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 2,
  "isChangesRequested": false
}

[typescript-bot]

🔔 @colbydehart @FourwingsY @jsiebern @mastermatt @pietrovismara @dawnmist @ycmjason @Aitor1995 @peterblazejewicz — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

[jameskirkwoodwsf]

Fantastic, thank you for fixing this!

[typescript-bot]

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#46290	diff
Batch compilation
Memory usage (MiB)	32.1	38.9	+21.3%
Type count	2752	2762	0%
Assignability cache size	221	221	0%
Language service
Samples taken	688	688	0%
Identifiers in tests	688	688	0%
getCompletionsAtPosition
Mean duration (ms)	92.8	90.0	-3.1%
Mean CV	14.0%	14.0%
Worst duration (ms)	141.3	129.2	-8.6%
Worst identifier	DateTime	toLocaleString
getQuickInfoAtPosition
Mean duration (ms)	91.1	88.4	-2.9%
Mean CV	13.6%	14.0%	+3.4%
Worst duration (ms)	131.3	119.8	-8.8%
Worst identifier	dtHebrew	name

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

[mastermatt]

Thanks.

Implementation: https://github.com/moment/luxon/blob/18ca968c6a326bd86edb28207b465fc189ae2edd/src/datetime.js#L1521-L1523

[peterblazejewicz]

Ready to merge 💋

[typescript-bot]

I just published @types/luxon@1.24.3 to npm.

[chrisdrackett]

how are people handling the migration to this? We can safely assume in our app that all inputs to luxon are valid dates, but now I need to add a ton of checks everywhere in my code I use it anyways? This is kinda a huge change.

[peterblazejewicz]

how are people handling the migration to this? We can safely assume in our app that all inputs to luxon are valid dates, but now I need to add a ton of checks everywhere in my code I use it anyways? This is kinda a huge change.

I'm not sure if v2 of luxon is anywhere near, as it increaments freuently with minor versions, so the proper handling could be problematic waiting for major release. Obviously, I personally can see the issue here with nulling everythin, especially if ?. cannot be used.
@mastermatt what's your opinion here, thx!

[mastermatt]

The values produced from Luxon have not changed in awhile. This PR simply fixed a bug in the types.

There might be something to do as far as having distinct interfaces for a datetime and a known valid datetime. Then the types for isValid could use type guards to force only the valid interface. But I'd really have to play around with options. I'm not sure, off the top of my head, if generics could be used in a useful way here.

[jthorn]

Found myself here after my build broke. I understand that this typing change may make things more "correct", but it seems to me that it is effectively a breaking API change.

Is there any way a change like this be reflected in the version number? (This assumes semantic versioning) ... I assume not since it appears that the @types version is intended to match the base library version.

[peterblazejewicz]

Is there any way a change like this be reflected in the version number? (This assumes semantic versioning) ... I assume not since it appears that the @types version is intended to match the base library version.

@thorn
Thanks for your input, it's important to have better picture.
I don't think it's feasable, see my comment, the luxon updates frequently but with minor upgrades, this would take long time before this fix could be added IMO.

@mastermatt
The public documentation most people use is out of the sync with code implementation IMO, this can be seen here:
https://moment.github.io/luxon/docs/class/src/datetime.js~DateTime.html#instance-method-toISO
https://moment.github.io/luxon/docs/file/src/datetime.js.html#lineNumber1520
IMO there could be a valid point the we went over the edge sligthly in the context of dev experience,
May we discuss reverting this change.
I'll make an issue or PR on the Luxon to fix the API which was subject of the change here.

[daviduzumeri]

Count me in on the risk/reward ratio here being a bit off. I totally understand that the typing issue here is absolutely more 'correct', but I'd be curious to know how many people this is helpful for vs. how many people this is just causing a bunch of extraneous checking code for.

[jthorn]

Thanks for your input, it's important to have better picture.
I don't think it's feasable, see my comment, the luxon updates frequently but with minor upgrades, this would take long time before this fix could be added IMO.

Thanks @peterblazejewicz, I found an "answer" to my own question about the versioning here ...
#34047

There are other discussions on the topic as well, so it seems that this is not an uncommon occurrence. But it's more a question for the DefinitelyTyped community at large. Even though each library owner is responsible for the content of their own sections of the repo, there is not a standard for handling breaking changes. The moral of the story for me is to not rely on SemVer for @types anymore and probably remove the caret versioning from my project on them.

[peterblazejewicz]

Iv'e made a PR with proposed revert, thanks!
#46322

[TomWeStartFires]

I may be biased here as I submitted the PR. But from my perspective although this is a breaking change, it is breaking because the types have been wrong from the beginning. So reverting it while fixing some builds would bring back the chances of type bugs (which may have always been there in the first place). That is to say it’s reverting a bug or reverting a implementation in the standard sense, it would cover up a bug that has always existed.

Ideally you would be able to bump the major version here. Possibly the idea of keeping in sync with the package while desirable is not compatible because of issues like these. A naive suggestion would be to have the version independent of the package and then have some kind of mapping that is published on build so that a developer can look up which @types version aligns with a package version.

Or another solution maybe publishing the package under a different name one way or another if possible ie @types/luxon-1.x.x and then have the types version being standard semantic versioning.

[jthorn]

I may be biased here as I submitted the PR. But from my perspective although this is a breaking change, it is breaking because the types have been wrong from the beginning.

@TomWeStartFires I don't think anyone is claiming that the code you created is "Wrong". However I think the fact that the Types have been wrong for a "long time" almost argues against making the breaking change. If the incorrect types were a large concern for the community, someone would have made this request long ago. Likewise, because the types have been wrong for so long, there is an increased chance of dependencies using the 'broken' types and coding to them as they are, which means more broken code when this change is made.

According to npmjs right now, this library has almost 200K downloads a week, which is not nothing. If it were a smaller library, and not widely used, this would probably be a non-issue.

As far as the versioning is concerned, it appears that the version standards are set for all DefinitelyTyped libraries so as to support easy discoverability with existing tooling. Simply changing the version strategy might result in package managers not being able to find the appropriate types for a library.

IMHO ... The 'Best' option would be to convince luxon to include the types in their distribution so that versioning becomes a non issue, and so that they can up the appropriate semantic version when a breaking change is made. But that's not likely to happen either.

All in all, I am not necessarily advocating for rolling this back. For people who pull the latest versions of libraries in their CI integrations, and build frequently, the damage has likely already been done. I, for one, have already accounted for the code change in my sources, but I do wonder how many others have been affected as well.

At least it's not on the scale of the whole left-pad debacle :)

[CarsonF]

I just hit this as well. For me, this change is "wrong" because I have

import { Settings } from 'luxon';
Settings.throwOnInvalid = true;

So in my codebase I'll never get nulls from these functions, an error will be thrown instead.

I don't have a good solution to this either. TS does not play well with having a global setting that changes types.
I would even argue against myself that this setting is opt-in so the the types here are more correct as nullable.

I can say for now I'll be locking this dependency to the previous version.

[thomas-darling]

Yeah, as much as I like correct typings, this change is an absolute pain in the ass, as these functions really should never return null in normal usage. I think it is reasonable to argue, that if the object tells you it is in an invalid state, then some things cannot be trusted to work - and as mentioned above, it can even be configured such that these functions will throw instead of returning null, and in that scenario, these updated typings are just plain wrong.

Please revert this.

[mastermatt]

I'm in favor of a revert. #46322 was opened, but it never passed CI. @peterblazejewicz did you have an issue on that one?

[peterblazejewicz]

@mastermatt I've rebased on master and pushed, it seems to fix CI (I guess the diffs were to large for CI). Fixed