serializr icon indicating copy to clipboard operation
serializr copied to clipboard

Constructor parameter decorators might not work correctly due to indeterministic initial schema setup

Open adamstoffel opened this issue 3 years ago • 1 comments

In a class that uses both constructor parameter decorators and property decorators such as:

class MyClass {
  constructor(
    @serializable(alias('b'))
    public b: string
  ) { }

  @serializable
  public a?: string;
}

If the decorator for property a is processed before the decorator for property b, the "special" factory function to call the constructor will not be setup in the ModelSchema. Consequently, the constructor will not be called when the class is deserialized.

adamstoffel avatar Jan 14 '22 01:01 adamstoffel

constructor parameter decorators aren't supported, so I recommend to declare the parameter as field first.

On Fri, Jan 14, 2022 at 1:01 AM Adam Stoffel @.***> wrote:

In a class that uses both constructor parameter decorators and property decorators such as:

class MyClass { constructor( @serializable(alias('b')) public b: string ) { }

@serializable public a: string;}

If the decorator for property a is processed before the decorator for property b, the "special" factory function to call the constructor will not be setup in the ModelSchema. Consequently, the proper class constructor will not be called with the class is deserialized.

— Reply to this email directly, view it on GitHub https://github.com/mobxjs/serializr/issues/160, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBGNYZFWV55ARI6HSYDUV5YW3ANCNFSM5L5M2JXQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you are subscribed to this thread.Message ID: @.***>

mweststrate avatar Jan 14 '22 09:01 mweststrate