Top Level aggregations in connections #5944
Conversation
🦋 Changeset detectedLatest commit: 5d36415 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
angrykoala
force-pushed
the
aggregation-connections
branch
from
60e7337 to
ef2d979
Compare
| @@ -138,4 +159,38 @@ export class CompositeConnectionReadOperation extends Operation { | |||
| return [...nodeFields, ...edgeFields]; | |||
| }); | |||
| } | |||
|
|
|||
| // NOTE: duplicate from ConnectionReadOperation | |||
| private transpileAggregation(context: QueryASTContext): { | |||
There was a problem hiding this comment.
Not sure if it will be right approach the proposal I'm saying but:
Can we expose this method from the aggregationField and avoid duplicity between the CompositeConnectionReadOperation and the ConnectionReadOperation
There was a problem hiding this comment.
TBH, I don't think that is the best option. Sure, the code is the same, but it is more coincidental than anything, as transpilation for compositeConnection doesn't necessarily have to be the same, and I'm hesitant to couple the nested element of the tree. I'd rather have this duplicate code than break the encapsulation on those
| await testHelper.executeCypher( | ||
| ` | ||
| CREATE (:${typeMovie} {testString: "${testString}", id: "1", title: "1", imdbRating: 1, createdAt: datetime("${minDate.toISOString()}")}) | ||
| CREATE (:${typeMovie} {testString: "${testString}", id: "22", title: "22", imdbRating: 2, createdAt: datetime()}) |
There was a problem hiding this comment.
Even if these tests were already made, in my opinion, it will cover more cases if some entries are filtered out by the testString filter!
There was a problem hiding this comment.
As you said, those cases are already covered. And these tests are based on the existing ones, Improving tests on aggregations is a bit out of scope for this, already quite big, PR. But I'll keep this in mind as we do need to improve these tests once the aggregations work is complete
Description
Add top level aggregations inside connection operations: