fix: Allow default initialization of DateTime/Duration/Interval

[mbStavola]

This PR default initializes the config parameter of the constructor for DateTime, Duration, and Interval in order to fix the following:

Cannot read properties of undefined (reading 'zone')

This can be encountered when using a third party serialization/deserialization library such as class-transformer as temporary objects may be constructed with new while waiting for any transformation logic to run. This is actually a very common issue for NestJS users.

Understandably, these constructors are meant to be private, but the unfortunate fact is that nothing can prevent new from being called on these types. Moreover, if you look at the constructor definitions it seems that the intent was to provide sane defaults already. With that in mind, it may make sense just to provide a default to sidestep the issue entirely and let the existing code handle things.

Ultimately, this is a minor adjustment which fixes some very real pain points.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: mbStavola / name: Matt Freitas-Stavola (edb4373, 278cee2)

[mbStavola]

I am also aware of this previous attempt, but I did not want to revive a year old dead PR. Additionally, that PR only covers DateTime.

[mbStavola]

@icambron just saw the force-push notification here, was wondering if you had some time to review this?

[icambron]

Hmm, do the objects you construct this way actually work? I'm sort of guessing you'll get errors using them because e.g. values is not defined...

[mbStavola]

@icambron Would it be better to instead have the constructor return invalid instances instead if a config is not supplied? That way construction doesn't throw an error, but the object returned is properly marked as invalid? For example, something like:

export default class DateTime {
  /**
   * @access private
   */
  constructor(config) {
    if (!config) {
      return DateTime.invalid('no config supplied');
    }

Something like this would still require that a config is supplied in order to create valid instances, but save some headaches for those using libraries which by default new these objects.

EDIT: I made the changes and did some testing, works rather well but I did notice that Locale had a similar issue as the other three classes. In that case I felt it might be better to default to the system locale when one wasn't provided, similar to the provided factory functions.

[mbStavola]

For those interested, here are some of the simpler test cases I ran to confirm the changes:

/* global test expect */

// Requires an npm i -D class-transformer
import { plainToInstance, instanceToPlain, Transform } from "class-transformer";

import { DateTime } from "../src/luxon";

export const IntoDateTimeTransformer = (
  { format, acceptNulls, ...opts } = {},
  options = {},
) => {
  return Transform(
    ({ value }) => {
      if (DateTime.isDateTime(value)) {
        return value;
      }

      return DateTime.fromISO(value, { setZone: true, ...opts });
    },
    { ...options, toClassOnly: true },
  );
};

export const FromDateTimeTransformer = (
  { format, acceptNulls, ...opts } = {},
  options = {},
) => {
  return Transform(
    ({ value }) => {
      if (!DateTime.isDateTime(value)) {
        throw new Error('Value is not a DateTime');
      }

      return value.toISO(opts);
    },
    { ...options, toPlainOnly: true },
  );
};

// This is what Typescript will codegen as a decorator helper function
// Stealing it to make our lives easier in testing
const __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
  var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
  if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
  else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
  return c > 3 && r && Object.defineProperty(target, key, r), r;
};

class Person {
  name;
  birthday;

  constructor(name, birthday) {
    this.name = name;
    this.birthday = birthday;
  }
}

__decorate([
  IntoDateTimeTransformer(),
  FromDateTimeTransformer(),
], Person.prototype, "birthday", undefined);

test("Instance to plain works", () => {
  const person = new Person('Bob', DateTime.fromISO('1990-10-12', { zone: 'UTC' }));
  const serialized = instanceToPlain(person);

  expect(serialized.name).toBe('Bob');
  expect(serialized.birthday).toBe('1990-10-12T00:00:00.000Z');
});

test("Plain to instance works (plain string)", () => {
  const person = { name: 'Alice', birthday: '1992-05-15T00:00:00.000Z' };
  const deserialized = plainToInstance(Person, person);

  expect(deserialized.name).toBe('Alice');
  expect(deserialized.birthday).toStrictEqual(DateTime.fromISO('1992-05-15', { zone: 'UTC' }));
});

test("Plain to instance works (datetime object)", () => {
  const person = { name: 'Eve', birthday: DateTime.fromISO('1991-08-03', { zone: 'UTC' }) };
  const deserialized = plainToInstance(Person, person);

  expect(deserialized.name).toBe('Eve');
  expect(deserialized.birthday).toStrictEqual(DateTime.fromISO('1991-08-03', { zone: 'UTC' }));
});

On the primary branch, that last test will fail due to the aforementioned issue. I did not commit this because I don't think its the responsibility of Luxon to test for class-transformer compatibility specifically, however I did include tests in this MR to ensure the default constructor works.

[mbStavola]

@icambron Small nudge, I know you're probably very busy but any chance you can take another look?