luxon icon indicating copy to clipboard operation
luxon copied to clipboard

DateTime.fromObject() tries to use properties that are not specified in DateObjectUnits

Open alienisty opened this issue 2 years ago • 4 comments

When creating a DateTime using the fromObject() factory method, the current implementation tries to normalize and use all own properties of the instance passed in as parameter, and if the instance defines other own properties, the method throws an 'Invalid unit' error.

To Reproduce

class NamedDate {
  constructor(readonly name: string,
              readonly year: number,
              readonly month: number,
              readonly day: number){}
}

const value: NamedDate = new NamedDate(...);
DateTime.fromObject(value);

Actual vs Expected behavior The above code will throw 'Invalid unit name' whereas the mothod should only try to use and normalize properties specified in DateObjectUnits and ignore any other property of the provided object.

Desktop

  • OS: Windows 10
  • Browser Chrome 102.0.5005.115 (Official Build) (64-bit)
  • Luxon version 2.4.0
  • Your timezone "Australia/Perth"]

alienisty avatar Jun 21 '22 04:06 alienisty

I disagree: if we made it more lenient, it would easy for people to get the keys wrong, because none of them are required. This is especially painful given that Luxon provides default values for the units. So if you did DateTime.fromObject({ y: 2022, month: 6, day: 21 }) it would look like everything worked fine, when it fact it's totally ignoring the y. Next year that code produces the wrong date.

icambron avatar Jun 21 '22 19:06 icambron

Arguably though, DateObjectUnits is an interface, so, at least in TypeScript, I would expect to be able to pass any implementation of that interface as parameter to the method.

Also, another thing that would make a lot of sense to me, but is prevented by the current implementation, is the following:

const aDateTime: DateTime = ...
const copy = DateTime.fromObject(aDateTime);

alienisty avatar Jun 22 '22 00:06 alienisty

You needn't ever copy a date time like that; they're immutable, so just use the original where you would use the copy. It's like how you never copy a string and there isn't really a way to do it.

icambron avatar Jun 22 '22 08:06 icambron

I am not saying I would, but it should be possible if I only looked at the types. DateTime is structurally compatible with DateObjectUnit. By the way, sometimes is useful to have instances that are equal but not same.

alienisty avatar Jun 22 '22 08:06 alienisty