Skip to content

Conversation

@cuchi
Copy link
Contributor

@cuchi cuchi commented Sep 13, 2018

When saving a new entity with repository.save(fields), the current type definition fails to return to me the right fields types for the generated columns, for example:

@Entity()
export default class User {
    @PrimaryGeneratedColumn('uuid') id: string
    @Column() name: string
    @Column({ nullable: true }) address?: string
}

// ...

const user = { name: 'John Doe', address: 'Somewhere' }
const createdUser = await this.repository.save(user)

console.log(createdUser.name) // okay 
console.log(createdUser.address) // okay
console.log(createdUser.id) // compilation error: Property 'id' does not exist on type ...

This seems to only make sense when reload: false is sent, which is not the case.
This little change fixes that, while overriding the optional type when the values are present.

I'm not sure if this breaks some special case, maybe some maintainer could look into this 😅

@pleerock
Copy link
Member

Thanks for contribution!

by making it & Entity it means we add all entity properties which is not truth either. Not sure if we should merge it.

@cuchi
Copy link
Contributor Author

cuchi commented Oct 22, 2018

by making it & Entity it means we add all entity properties which is not truth either. Not sure if we should merge it.

That's why I define those nullable properties as TypeScript optional types. See the prop address in my example.

With my change, the following is true, because { foo: type } & { foo?: type } still resolves to { foo: type }

@Entity()
export default class User {
    @PrimaryGeneratedColumn('uuid') id: string
    @Column() name: string
    @Column({ nullable: true }) address?: string
    @Column({ nullable: true }) age?: number
}

// ...

const user = { name: 'John Doe', address: 'Somewhere' }
const createdUser = await this.repository.save(user)

createdUser.address // okay
createdUser.age // error, might be undefined

@pleerock pleerock merged commit 00e6a39 into typeorm:master Nov 24, 2018
@pleerock
Copy link
Member

okay, lets merge it, and lets see how people react to this change. Thank you for the contribution!

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.

2 participants