Skip to content

Conversation

@flodlc
Copy link
Contributor

@flodlc flodlc commented May 14, 2022

The current ModelObject does not preserve ? optional properties.

Example:

export class User extends Model {
  id: string;
  name: string;
  parentId?: User
}

function insertUser(user: Omit<ModelObject<User>, "id">) {...}

// Typescript will throw an error here. Missing property parentId
insertUser({
    name: "john"
});

// Unfortunately, I have to manually set parentId: undefined
insertUser({
    name: "john",
    parentId: undefined
});

It's not surprising since with this declaration we lose the ? optional syntax

type ModelObject<T extends Model> = {
    [K in DataPropertyNames<T>]: T[K];
};

Typescript has a native Pick<Type, Keys> utility Type for this which preserves optional properties and allows a shorter syntax.

Not sure if it can be used also for PartialModelObject
What do you think ?

@koskimas
Copy link
Collaborator

This is the implementation of Pick

type Pick<T, K extends keyof T> = {
    [P in K]: T[P];
};

which is 100% the same as what we have.

@koskimas koskimas closed this May 17, 2022
@flodlc
Copy link
Contributor Author

flodlc commented May 17, 2022

This is the implementation of Pick

type Pick<T, K extends keyof T> = {
    [P in K]: T[P];
};

which is 100% the same as what we have.

@koskimas
It's not the same.
The difference is that the original Pick extends keyof T so it keeps all the keys information of the given keys instead of only iterating on the given key names.

type Pick<T, K extends keyof T> = {
    [P in K]: T[P];
}
// Pick keys extends keyof T so it keeps `?`

Some examples with a simple User

Capture d’écran 2022-05-17 à 10 07 54

Capture d’écran 2022-05-17 à 10 08 40

Capture d’écran 2022-05-17 à 10 02 35

@posgarou
Copy link

@koskimas Can this be reopened and considered? @flodlc's solution fixes some type issues I've run into.

The snippet below fails to compile with the current definition of ModelObject, but passes with the proposal from @flodlc.

class Cat extends Model {
  age?: number;
}

type CatObject = ModelObject<Cat>;

const cat = Cat.fromJson({})
const catObject: CatObject = cat;
// Error: Type 'Cat' is not assignable to type 'CatObject'.
// Property 'age' is optional in type 'Cat' but required in type 'CatObject'

@lehni
Copy link
Collaborator

lehni commented Jun 30, 2023

I'm going with @falkenhawk's suggestion here and go ahead and merge this: #2299 (comment)

@lehni lehni merged commit 6420a8e into Vincit:master Jun 30, 2023
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

Successfully merging this pull request may close these issues.

4 participants