RicoSuter / NJsonSchema

JSON Schema reader, generator and validator for .NET
http://NJsonSchema.org
MIT License
1.4k stars 535 forks source link

typescript: field initialization order in derived classes #625

Open wuzzeb opened 6 years ago

wuzzeb commented 6 years ago

With c# code such as

public class Bar {
  public int bar {get;set;}
}

public class BarChild : Bar {
  public int[] values {get;set;}
}

the generated typescript will look something like

export class Bar implements IBar {
  bar: number;

  constructor(data?: IBar) {
    if (data) {
        for (var property in data) {
            if (data.hasOwnProperty(property)) {
                (<any>this)[property] = (<any>data)[property];
            }
        }
    }
  }

  // ... rest of Bar class
}

export class BarChild extends Bar implements IBarChild {
    values: number[] = []; // <------ note the initializer here

    constructor(data?: IBarChild) {
        super(data);  // <--- call to base class constructor
    }

   // ... rest of BarChild class ...

The BarChild constructor causes incorrect data initialization. The rules for typescript classes (and eventually javascript classes once emcascript adds property initializers) is as follows:

  1. The base class initialized properties are initialized
  2. The base class constructor runs
  3. The derived class initialized properties are initialized
  4. The derived class constructor runs

applying these rules to the above, what happens is the base class Bar initialized properties are run (there aren't any), the Bar constructor runs (which copies the properties from data into the new instance), the derived class (BarChild) properties are initialized (in this case, values is set to [] overwriting the value copied from data), and finally the derived class constructor runs (which does nothing). Therefore, no matter what you pass to the BarChild constructor, values will always be the empty list since the initializer overwrites anything from the base class constructor.

You can read more about this at https://github.com/Microsoft/TypeScript/issues/10634 or https://github.com/Microsoft/TypeScript/issues/1617 or https://github.com/Microsoft/TypeScript/issues/13525 Also, https://stackoverflow.com/questions/43595943/why-are-derived-class-property-values-not-seen-in-the-base-class-constructor/43595944 contains a discussion why this is the case.

I was looking through https://github.com/RSuter/NJsonSchema/blob/master/src/NJsonSchema.CodeGeneration.TypeScript/Templates/Class.liquid but I don't see an easy way of fixing... the problem is everything about the base class runs before the derived class even gets a chance to do anything. One option would be to not use property initialization at all. Instead, the BarChild constructor could contain code as

export class BarChild extends Bar implements IBarChild {
  values: number[]; // <--- no initializer

 constructor(data?: IBarChild) {
    super(data);
    if (values === undefined) { // <-- initializer here
       values = [];
    }
  }
}
RicoSuter commented 6 years ago

This is pretty serious, thanks for reporting.

I think only your last option would be viable... but it would look like that:

export class BarChild extends Bar implements IBarChild {
  values: number[]; // <--- no initializer

 constructor(data?: IBarChild) {
    super(data);
    if (!data) { // <-- defaults only if no data is provided
       this.values = [];
    }
  }
}

Wouldn't this produce much more code?

RicoSuter commented 6 years ago

Can you have a look at my commit? Does it look good? Do you think the issue is fixed with this change?

rahulbpatel commented 4 years ago

Based on this suggestion, as a workaround I've gone with this

export abstract class ModelBase<T> {

    /**
     * Creates new initialized instance of model
     */
    static new<T extends ModelBase<T>>(this: new () => T, data: any): T {
        const result = new this();

        result.init(data);

        return result;
    }

    protected init(init?: Partial<T>) {
        Object.assign(this, init);
    }
}

export class User extends ModelBase<User> {
   id = 1;
   customInitialized: string;
   protected init(data?: any) {
        super.init(data);

        if (data.something) {
           this.customInitialized = 'foobar';
        }
    }   
}

User.new({id: 2, something: true});