-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Aggregated optimization changes #2744
Aggregated optimization changes #2744
Conversation
…rate call and even move with many relations
…eat/aggregated-prs
@michaelbromley In this PR I will optimize queries inside TransactionalConnection, it turned out to be difficult because there are assets inside collections that don't have reverse relation. We need to figure something out, maybe this relation can be found through metadata
|
…lations without any eager relations
…rseSide relation for list-query-builder
@michaelbromley
We are just a little bit away from the final optimization of the most important methods
In overview:
Also, this PR includes fixes that address this issue #2738 |
how progress of this PR :) |
When i will be available
|
# Conflicts: # packages/core/src/service/helpers/entity-hydrator/entity-hydrator.service.ts # packages/core/src/service/helpers/list-query-builder/list-query-builder.ts # packages/core/src/service/services/channel.service.ts
…th of eager relations
PR is ready 🚀 |
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.
Excellent! Just a couple of small "tidy up" points. Thanks again for this incredible effort.
* has already been joined to avoid adding duplicate join statements. | ||
* @returns boolean Returns true if the relation has already been joined (based on the alias), false otherwise. | ||
* @template T extends VendureEntity The entity type for which the query builder is configured. | ||
*/ |
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.
Looks like this doc block is out of place?
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.
Yeah, I don't know how to do it here. If you have recommendations, I'd be glad to change it and maybe I need to reduce the amount of text...
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.
Looks like this doc block is out of place?
I deleted the documentation that was accidentally copied and updated the documentation I was adding for the method. Check it out, please.
@@ -24,7 +24,7 @@ export abstract class OrderableAsset extends VendureEntity implements Orderable | |||
assetId: ID; | |||
|
|||
@Index() | |||
@ManyToOne(type => Asset, { eager: true, onDelete: 'CASCADE' }) | |||
@ManyToOne(type => Asset, { eager: true, onDelete: 'CASCADE' }) |
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.
extra space
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.
ok, I will fix it
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.
extra space
done
} from 'typeorm'; | ||
import { DataSource } from 'typeorm/data-source/DataSource'; |
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.
should this (and the InjectDataSource
above) really be deep imports?
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.
ok, I will fix it. this is from my IDE, I didn't noticed
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.
done
should this (and the
InjectDataSource
above) really be deep imports?
48b239b
into
vendure-ecommerce:minor
Related issue: #2738