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

Collection class constructors (implications of only allowing no-arg ctor's) #117

Open
GitTom opened this issue Nov 12, 2019 · 6 comments
Open

Comments

@GitTom
Copy link

GitTom commented Nov 12, 2019

FireORM requires that my collection class's constructor have no parameters, so I can't pass in the data needed to initialize the class. This means that I have to add the 'undefined' type to all my class attributes, and initialize them after the class has been created, otherwise TS will complain (assuming TS strict mode).

But 'undefined' is an invalid value in the datastore. If an entity doesn't have a value for an attribute it needs to set it to null, not undefined, so I have to add null as a valid type for each non-mandatory attribute too.

But even then, those attributes get written to the DS (with value 'null') so the potential to save storage by omitting unused values is lost.

Have I got this right?

If I do, here are some thoughts/suggestions... would it be possible for

  • Fireorm to allow constructor parameters? (The parameters would be undefined when the constructor is invoked by fireform and the constructor implementation would have to handle that.)

  • for undefined values to not be written to the datastore (rather than causing an error). BTW, the error message I got for this was very informative - thanks!.

@wovalle
Copy link
Owner

wovalle commented Nov 19, 2019

Thanks for the suggestions!

I wasn't aware about this. I'm aware that parameterless constructors are required so fireorm can initialize the instance for you but I didn't know that you couldn't declare your own constructor with parameters (although you have to keep the parameterless one for fireorm).

I'll definitely check this out.

@GitTom
Copy link
Author

GitTom commented Nov 19, 2019

Yes, javascript doesn't allow overloading for function/method/constructor parameters. And I believe that having one signature with no parameters and one with, would still be considered overloading.

Perhaps the loose nature of both the type system and the parameter system makes it impossible.

Anyway, thanks for consider my feedback. I don't mind whether you close this issue or keep it open.

@zhirschtritt
Copy link
Contributor

@GitTom As a workaround, I would suggest adding a manufacture method to your collection classes that returns a new instance (or existing instance) of your class.

@skalashnyk
Copy link
Contributor

@GitTom , it's possible to have several definitions, something like this:

class Foo {
  id: string = null;
  name: string;
  age: number;

  constructor();
  constructor(name: string);
  constructor(age: number);
  
  constructor(nameOrAge?:string|number) {
    // 
  }
}

@GitTom
Copy link
Author

GitTom commented Nov 28, 2019

@skalashnyk Interesting. I guess that is a Typescript attempt to work around the limitation. Seems a bit awkward though, and I prefer not to stray that far from JS when I write TS.

@skalashnyk
Copy link
Contributor

skalashnyk commented Nov 29, 2019

@skalashnyk Interesting. I guess that is a Typescript attempt to work around the limitation. Seems a bit awkward though, and I prefer not to stray that far from JS when I write TS.

As I know, it's an only way, how to overload function/method in typescript and many vendors implement their libraries this way...

For example, class-transformer that fireorm uses to serialize models:

/**
 * Converts plain (literal) object to class (constructor) object. Also works with arrays.
 */
export function plainToClass<T, V>(cls: ClassType<T>, plain: V[], options?: ClassTransformOptions): T[];
export function plainToClass<T, V>(cls: ClassType<T>, plain: V, options?: ClassTransformOptions): T;
export function plainToClass<T, V>(cls: ClassType<T>, plain: V|V[], options?: ClassTransformOptions): T|T[] {
    return classTransformer.plainToClass(cls, plain as any, options);
}

https://github.com/typestack/class-transformer/blob/develop/src/index.ts#L31

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

No branches or pull requests

4 participants