Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't use deserializeObject if that class has a manual a constructor method implemented. #47

Open
magtastic opened this issue Dec 21, 2017 · 8 comments
Assignees
Labels
question Further information is requested

Comments

@magtastic
Copy link

I have this class that looks something like this:

@JsonObject
export class UserPublicProfile implements RefBalrogModel {
  @JsonProperty('timestamp', TimestampConverter)
  timestamp: Timestamp = new Timestamp();

  @JsonProperty('user_name', String)
  user_name: string = undefined;

  @JsonProperty('user_id', UserIDConverter)
  user_id: UserID = undefined;

  @JsonProperty('display_name', String)
  display_name?: string = undefined;

  public static reference = DatabaseReference.publicProfile;

  constructor(user_id: UserID, user_name: string, timestamp: Timestamp, display_name?: string) {
    this.timestamp = timestamp;
    this.user_id = user_id;
    this.user_name = user_name;
    this.display_name = display_name;
  }
}

and when trying to deserialize a json object to that UserPublicProfile I get the following error:
Argument of type 'typeof UserPublicProfile' is not assignaable to parameter of type 'new () => any'.

I want to be able to initialize the object with parameters, and a jsonObject. Is that not possible with this package?

@andreas-aeschlimann
Copy link
Member

You are right, it does not work that way and we should note that in the ReadMe.

So the problem is the following: When deserializing JSON to the class, json2typescript has to create a new instance of your class. This happens dynamically, so it is logical that you cannot have a constructor like that. Or how would json2typescript know the values of the parameters?

I suggest the following options:

  1. Do not use a classical constructor, use a "fake" overload constructor. Example:
constructor() {}
createUserPublicProfile(user_id: UserID, user_name: string, timestamp: Timestamp, display_name?: string): UserPublicProfile {
    const userPublicProfile = new UserPublicProfile();
    userPublicProfile.timestamp = timestamp;
    userPublicProfile.user_id = user_id;
    userPublicProfile.user_name = user_name;
    userPublicProfile.display_name = display_name;
    return userPublicProfile;
}
  1. Make all params optional
constructor(user_id?: UserID, user_name?: string, timestamp?: Timestamp, display_name?: string) {
    this.timestamp = timestamp;
    this.user_id = user_id;
    this.user_name = user_name;
    this.display_name = display_name;
}

Overall, you should make sure the values are properly typed. It looks like display_name is optional, but in your JSON you did not declare that in the @JsonProperty decorator; instead you set it to string.

@andreas-aeschlimann andreas-aeschlimann self-assigned this Dec 23, 2017
@andreas-aeschlimann andreas-aeschlimann added the question Further information is requested label Dec 23, 2017
@valoricDe
Copy link

Using https://github.com/weichx/cerialize you can define also a non empty constructor. Maybe it is possible to the idea from there.

@andreas-aeschlimann
Copy link
Member

Using https://github.com/weichx/cerialize you can define also a non empty constructor. Maybe it is possible to the idea from there.

Interesting, thank you for the link.

@oayoub84
Copy link

oayoub84 commented May 4, 2022

Hi, i have the same problem but with Dependency Injection, so i cannot use neither optional args nor fake method. I think it is better to pass an instance of the class instead of a reference. That way we can avoid all sort of problems.

@andreas-aeschlimann
Copy link
Member

Hi, i have the same problem but with Dependency Injection, so i cannot use neither optional args nor fake method. I think it is better to pass an instance of the class instead of a reference. That way we can avoid all sort of problems.

Does #47 (comment) not fix it?

@oayoub84
Copy link

oayoub84 commented May 4, 2022

No, the constructor args are mandatory.
I can make a PR, if you agree.

@andreas-aeschlimann
Copy link
Member

How do you mean "mandatory". By the DI system?

Yes you can make a PR if you like, but in order to spare us a misunderstanding, I suggest you describe first how you plan to implement it or what you plan to change.

@oayoub84
Copy link

This is an example in one of our projects where we use Json2Typescript.

import { JsonObject, JsonProperty } from 'json2typescript';

@JsonObject('Offer')
export class Offer {
  @JsonProperty('id', String, true)
  id: string = '';

  @JsonProperty('name', String, true)
  name: string = '';

  @JsonProperty('description', String, true)
  description: string = '';

  @JsonProperty('minAmount', Number, true)
  minAmount: number = 0;

  @JsonProperty('maxAmount', Number, true)
  maxAmount: number = 0;

  get title(): string | undefined {
    if (this.minAmount && this.minAmount > 0 && this.maxAmount && this.maxAmount > 0) {
      return this.translateService(`TITLE_1`, { arg1: 'arg1', arg2: 'arg2'});
    }

    if (!this.minAmount && !this.maxAmount) {
        return this.translateService(`TITLE_2`, { arg1: 'arg1', arg4: 'arg4'});
    }

    if (this.minAmount && this.minAmount > 0) {
        return this.translateService(`TITLE_3`, { arg3: 'arg3'});
    }

    if (this.maxAmount && this.maxAmount > 0) {
        return this.translateService(`TITLE_4`, { arg5: 'arg5', arg6: 'arg6', arg4: 'arg7' });
    }

    return;
  }

  constructor(private translateService: TranslateService) {}
}

Where we can't pass the translateService to the title(), because its used more than 15 times in code (angular templates). And adding a function to the class juste for injecting translateService, isn't a good solution because the property will be optional and the compiler will not rise an error if we forgot to call the function.

On general what i'm proposing is adding another functions for deserializing using instances instead of class reference. some thing like deserializeObjectByInstance(Object, Class Instance)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants