[luxon] Minor fixes and improvements

[hugofpsilva]

Luxon

DateTime

Added overloads for local() and utc(). (Are now necessary due to the object param after the number params).
Marked several params as optional where Luxon doesn't throw errors.
Fixed typing errors in ordinal getter, resolvedLocaleOptions, toLocaleString, toLocaleParts, and all format presets.

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.
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm test <package to test>.

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/api-docs/index.html

[typescript-bot]

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

1 package in this PR

• luxon — on npm, on unpkg (author is owner)
  • owner-approval: @peterblazejewicz

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.

You can test the changes of this PR in the Playground.

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": 55443,
  "author": "hugofpsilva",
  "headCommitOid": "4c3db3e8b22bacbbcc6b8c80dcfff500276b0234",
  "lastPushDate": "2021-08-29T17:26:07.000Z",
  "lastActivityDate": "2021-08-29T19:49:50.000Z",
  "mergeOfferDate": "2021-08-29T19:44:04.000Z",
  "mergeRequestDate": "2021-08-29T19:49:50.000Z",
  "mergeRequestUser": "hugofpsilva",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "popularityLevel": "Popular",
  "pkgInfo": [
    {
      "name": "luxon",
      "kind": "edit",
      "files": [
        {
          "path": "types/luxon/src/datetime.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/luxon/test/luxon-tests.global.ts",
          "kind": "test"
        },
        {
          "path": "types/luxon/test/luxon-tests.module.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "colbydehart",
        "FourwingsY",
        "jsiebern",
        "mastermatt",
        "pietrovismara",
        "dawnmist",
        "ycmjason",
        "Aitor1995",
        "peterblazejewicz",
        "carsonf",
        "hugofpsilva"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "peterblazejewicz",
      "date": "2021-08-29T19:43:30.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 907598238,
  "ciResult": "pass"
}

[typescript-bot]

🔔 @colbydehart @FourwingsY @jsiebern @mastermatt @pietrovismara @dawnmist @ycmjason @Aitor1995 @peterblazejewicz @CarsonF — 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.

[peterblazejewicz]

one topic at time, why those should be optional? Is that how the Luxon states this (in Docs)?
There is no single test-case e.g. for fromJSDate that covers no input.
I'd be cautious with such hanges, I've to fix a runtime by introduced by minor change in markdown-it recently, when undefined was just added to DT as proper input a month ago. There was no single test in native JS package to cover that scenario, so code started failing at runtime when people used 'undefined' as a valid value.
I'd say Luxon docs are clear to say 'required' for those params (assuming we are using JSDocs convention)

[hugofpsilva]

You are again correct, the docs don't specify these arguments as optional. Then again, the docs aren't nearly 100% accurate. Just as a fast example, the local() documentation doesn't even mention the options object as a last parameter (but does show it in the examples).

IMO Luxon is very permissive with the values passed to some (not all) of its methods.

In the example you gave of fromJSDate, Luxon checks if the date param is a JS Date with
Object.prototype.toString.call(date) === "[object Date]";
which, although far from perfect, by the spec available here should return false for undefined as Object.prototype.toString.call(undefined) returns "[object Undefined]".

I also added type tests for all the methods for which I marked the arguments as optional.

Don't get me wrong, I agree with you that DateTime.fromJSDate() with no arguments makes no sense, and the others I changed make no sense as well, but it is indeed accepted by the source code and it does return a DateTime, although an invalid one, instead of throwing an error. If the source allows it, why shouldn't we?

[CarsonF]

I agree with @peterblazejewicz here. Just because a JS library doesn't break with a certain input doesn't mean it's expected/"allowed". As you said, calling from* without an input doesn't make sense. Therefore TS can help enforce this, even beyond what the author has expressed / coded in runtime checks.

[hugofpsilva]

Alright it actually makes a lot more sense to me as it was, so I'm down for keeping it, I just thought it wasn't TS's job to restrict the user beyond typechecking.

I'll roll back that part 😃

[typescript-bot]

@peterblazejewicz Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

[peterblazejewicz]

LGTM!

[hugofpsilva]

Ready to merge

[typescript-bot]

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

[mister-what]

This type is inherently broken.
The type allows:

DateTime.fromISO("2021-09-13T07:52:27.697Z").toLocaleString({
  ...DateTime.DATETIME_MED,
  hour: "short"
})

But crashes at runtime. DateTimeFormatPresetValue does not apply for all options in DateTimeFormatPreset.

[joshuajaco]

It is not restrictive enough in some cases and too restrictive in others.
For example the following throws a type error but works correctly at runtime:

DateTime.fromISO("2021-09-13T07:52:27.697Z").toLocaleString({
  ...DateTime.DATETIME_MED,
  hour: "2-digit"
})

[peterblazejewicz]

to be eventually fixed here: #55750

[joshuajaco]

Why was this type changed at all? Shouldn't the presets always be valid DateTimeFormatOptions as well?

[peterblazejewicz]

I believe the intent was to provide better typing for presets. I've read a book, the author rewrote ending 39 times, this applies to our code IMO
#55750