-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
feat: embedded entities for entity schema #6318
Conversation
Allows to define embedded entities when using EntitySchema Closes: #3632
@@ -53,7 +54,7 @@ export class EntityMetadataBuilder { | |||
// ------------------------------------------------------------------------- | |||
|
|||
constructor(private connection: Connection, | |||
private metadataArgsStorage: MetadataArgsStorage) { | |||
private metadataArgsStorage: MetadataArgsStorage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file has much of redundant formatting...
```ts | ||
import {EntitySchema} from "typeorm"; | ||
|
||
const AddressEntity = new EntitySchema({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since technically entity
is an equivalent of a database table
, it's better to call it AddressEmbed
to avoid confusion.
embeddeds: { | ||
address: { | ||
isArray: false, | ||
type: () => AddressEntity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to have an ability to specify a name too (by embed name instead of variable reference)
/** | ||
* Type of the class to be embedded. | ||
*/ | ||
type: ((type?: any) => Function | EntitySchema); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Function
can be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I understood it can be a non-entity-schema entity
@@ -556,30 +557,57 @@ export class EntityMetadataBuilder { | |||
protected createEmbeddedsRecursively(entityMetadata: EntityMetadata, embeddedArgs: EmbeddedMetadataArgs[]): EmbeddedMetadata[] { | |||
return embeddedArgs.map(embeddedArgs => { | |||
const embeddedMetadata = new EmbeddedMetadata({ entityMetadata: entityMetadata, args: embeddedArgs }); | |||
const targets = MetadataUtils.getInheritanceTree(embeddedMetadata.type); | |||
if (typeof embeddedMetadata.type === "function") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need these changes? Basically, we shouldn't change this file and only EntitySchemaTransformer
should be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I wrote below, we need to make embeddedMetadata.type equal to Function | undefined then we can rewrite this code just a bit and avoid duplication.
@@ -32,7 +33,7 @@ export class EmbeddedMetadata { | |||
/** | |||
* Embedded target type. | |||
*/ | |||
type: Function; | |||
type: Function | EntitySchema; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what we should do instead is to mark type
as optional, and don't store anything here if type is entity schema is used.
@@ -190,7 +191,7 @@ export class EmbeddedMetadata { | |||
* Creates a new embedded object. | |||
*/ | |||
create(): any { | |||
return new (this.type as any); | |||
return typeof this.type === "function" ? new (this.type as any) : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here we'll have a check for type not being undefined.
|
||
/** | ||
* Type of the class to be embedded. | ||
*/ | ||
type: ((type?: any) => Function); | ||
type: ((type?: any) => Function | EntitySchema); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets make it undefined if entity schema is used.
Great additional to entity schemas, thank you |
Hi @pleerock . |
Currently needing this myself as I'm trying to implement clear separations in a DDD styled project. Thanks @igoraguiar and @pleerock |
This comment has been minimized.
This comment has been minimized.
Can we help anyhow ? |
If you make the changes requested & open a new PR I will close this one and review yours. |
any progress in it? from my perspective, it's the most important feature which ought to have kept up with embedded entities feature (e.g. by means of decorators) |
I have to close this PR because work is not finished and its abandoned. Please feel free to create a new PR with required changes when you'll have a time. |
Allows to define embedded entities when using EntitySchema
Closes: #3632