discussion: custom class constructor

[tonyxiao]

How should we handle situations where there are custom constructor for classes? E.g. if the class object I'm transforming into needs access to a database connection and I would like that to be passed into the constructor. Is it possible to still use plainToClass?

[NoNameProvided]

I don't know any way to do that at the moment.

[axiac]

plainToClass() should not use new to instantiate the objects it creates. If the constructor has arguments and validates them, using new className() without arguments triggers false validation errors. (I'm not talking about the class-validator Npm package here, I'm talking about custom code one puts in the constructor).

plainToClass() can use Reflection to create an object of the desired type without calling its constructor:

newValue = Reflect.construct(Object, [], targetType);

in TransformOperationExecutor.ts line 119 (and probably in other places where it creates objects using new).

[TimoGlastra]

@NoNameProvided I see you removed the help wanted label and added the discussion label.

Is this change still open for discussion or would you be willing to accept a PR? We use a lot of custom constructors and we now use the following pattern to avoid constructor errors:

class Test {
    constructor(options: TestType) {
        if (options) {
           // ... do things with constructor values
        }
}

This is suboptimal. Changing the internals to not call the constructor would be a big improvement for our project, so I'll be willing to invest the time.

[axiac]

Using the constructor to create an object in plainToClass() is semantically incorrect. The role of the constructor is to initialize a new object. plainToClass() restores (from its properties) an object whose properties were extracted by classToPlain(). A common use case for this scenario is to get the object properties (classToPlain()), serialize and store them in the database or send them through an HTTP request then restore from these properties an object identical to the original. classToPlain() captures the state of the object then plainToClass() uses the state to restore the object. The life of the restored object should not start from being created with new but from the point where the properties of the original object were captured.

[TimoGlastra]

@axiac looking into this. There is only one place where new is called, so changing it was easy.

However I'm running into some problems related to default properties. The repo recently had a PR merged that will keep default properties in the class. e.g.:

class Test {
    propertyWithDefaultValue = 10
}

Because the constructor isn't called the default values aren't set. Would you know how to deal with this?

Failing tests

 FAIL  test/functional/default-values.spec.ts
  ● expose default values › should set default value if nothing provided

    expect(received).toEqual(expected) // deep equality

    - Expected  - 5
    + Received  + 3

    - Object {
    -   "adminWithDefault": false,
    + User {
        "age": undefined,
    -   "ageWithDefault": 18,
    +   "ageWithDefault": undefined,
        "firstName": undefined,
    -   "firstNameWithDefault": "default first name",
    -   "lastNameWithDefault": "default last name",
    +   "firstNameWithDefault": undefined,
      }

      33 | 
      34 |     expect(transformedUser).toBeInstanceOf(User);
    > 35 |     expect(transformedUser).toEqual({
         |                             ^
      36 |       age: undefined,
      37 |       ageWithDefault: 18,
      38 |       firstName: undefined,

      at Object.<anonymous> (test/functional/default-values.spec.ts:35:29)

  ● expose default values › should take exposed values and ignore defaults

    expect(received).toEqual(expected) // deep equality

    - Expected  - 3
    + Received  + 1

    - Object {
    -   "adminWithDefault": false,
    + User {
        "age": NaN,
        "ageWithDefault": NaN,
        "firstName": undefined,
        "firstNameWithDefault": undefined,
    -   "lastNameWithDefault": "default last name",
      }

      48 | 
      49 |     expect(transformedUser).toBeInstanceOf(User);
    > 50 |     expect(transformedUser).toEqual({
         |                             ^
      51 |       age: NaN,
      52 |       ageWithDefault: NaN,
      53 |       firstName: undefined,

      at Object.<anonymous> (test/functional/default-values.spec.ts:50:29)

[axiac]

Not calling the constructors is a breaking change. I'm sure it breaks the functionality of many programs that use this package.

I would implement this change as an experimental feature that is disabled by default and can be enabled somehow by those who want to use it. This way it does not break the programs that use this package and can be released as a non-breaking new feature. From the semantic versioning point of view, this means an increase of the minor version.

The default behaviour can be changed to not call the constructors on the next major version of the package (the breaking changes require increasing the major version). Calling the constructor should still be available on the next major version as an opt-in, to allow the users of the package to upgrade to the new version without forcing them to use this new breaking-change feature.

Then, on the next major version (or after some time), the flag and the code that calls the constructors can be removed and the new behaviour becomes the only working way.

This is usually the flow to implement such a breaking change while keeping the backward compatibility with the previous versions of the package and letting the users of the package some time to accommodate the change into their code.

I recommend this approach because the changed behaviour is not visible in the interface of the package. The change is subtle and its effects might not be immediately visible.
In this example, the class does not have an explicit constructor declared in TypeScript, therefore it seems that not calling the constructor does not change the behaviour of the code. However, because of the properties with default values, the TypeScript compiler generates a constructor for the class (that initializes this.propertyWithDefaultValue) and not calling it changes the behaviour of the code.
Also, I'm sure most of the code in the wild that use this package either rely on the constructor being called or the developers do not realize that it is called and take its effect for granted.