fix: Allow default initialization of DateTime/Duration/Interval #1643
Conversation
|
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 |
|
@icambron just saw the force-push notification here, was wondering if you had some time to review this? |
|
Hmm, do the objects you construct this way actually work? I'm sort of guessing you'll get errors using them because e.g. |
|
@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 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. |
DateTime, Duration, and Interval constructors now return invalid when no config is supplied. This should fix any code which relies on default new while still keeping that invariant that configs are supplied.
dfbd8d9
to
278cee2
Compare
|
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. |
|
@icambron Small nudge, I know you're probably very busy but any chance you can take another look? |
This PR default initializes the config parameter of the constructor for
DateTime,Duration, andIntervalin order to fix the following:This can be encountered when using a third party serialization/deserialization library such as class-transformer as temporary objects may be constructed with
newwhile 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
newfrom 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.