chore(print): improve performance for data loading #2571
Conversation
| @@ -41,7 +42,8 @@ | |||
| denormalizationContext: ['groups' => ['write']], | |||
| normalizationContext: ['groups' => ['read']], | |||
| )] | |||
| #[ApiFilter(SearchFilter::class, properties: ['parent', 'contentType', 'root'])] | |||
| #[ApiFilter(SearchFilter::class, properties: ['contentType', 'root'])] | |||
There was a problem hiding this comment.
Why remove the parent filter? Are you sure that it wasn't used anywhere? I can't remember, but there were a lot of tests for this which you had to remove.
There was a problem hiding this comment.
Removing the parent filter results in children property to not be a single link but an array of links (basically disabling RelatedCollectionLinkNormalizer). This avoids a lot of above API Calls, because once I have the complete list of ContentNodes I can freely traverse down the child nodes without additional API calls.
hal-json-vuex is now "immune" to this, because it will just create a virtual key for the children array.
It shouldn't break anything. Just notice that in the frontend, we're actually no using the children property at the moment, because we do the filtering manually:
https://github.com/ecamp/ecamp3/blob/devel/frontend/src/components/activity/DraggableContentNodes.vue#L67
There was a problem hiding this comment.
Ah, now I get it. In this specific case (recursive data structure) we actually prefer the array of hrefs children: [{ href: '/content-nodes/1a2b3c4d' }, { href: '/content-nodes/1234abcd' }, ... ] over the shortened, filtered single link children: { href: '/content-nodes?parent=/content-nodes/00000000' }, because we don't want to embed all children, and because the array contains all the information we need for filtering in the frontend, without sending a separate request for /content-nodes?parent=/content-nodes/00000000
When I wrote the RelatedCollectionLinkNormalizer, I did not foresee that the link array could ever be more useful than the single filtered link. Maybe we don't need the parent filter right now. But I think we should introduce a way to explicitly disable the filtered link even when the filter exists. Otherwise, someone might in the future e.g. add filters for all relations, and make the performance worse without noticing.
There was a problem hiding this comment.
TODO: Explicitly comment in tests why children needs to be an array (to avoid someone adding the parent filter again in the future)
| @@ -24,7 +24,7 @@ | |||
| #[ApiResource( | |||
| collectionOperations: [ | |||
| 'get' => [ | |||
| 'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles', 'Activity:ContentNodes']], | |||
| 'normalization_context' => ['groups' => ['read', 'Activity:ActivityResponsibles']], | |||
There was a problem hiding this comment.
This was added to make the react-print load time bearable during development. So we should soon add similar replacements for this to react-print as you did for nuxt-print.
There was a problem hiding this comment.
Yes, absolutely. I suggest we merge this first, and then we can use the same data loading concept for react-print (by me or you - both ok for me).
Fixes the open todo from ecamp#2571 (comment)
Fixes the open todo from ecamp#2571 (comment)
Based on #2562, review only the new commits: https://github.com/ecamp/ecamp3/pull/2571/files/61343a10e8b066b9afbe50cc922e8c61d34d3385..69e96749c49fa629a248c0d9d1b37646a092e596
$contentNode->childrenas array of links to avoid recursive API callsToDo
API calls now (standard print config; Harry-Potter-Lager)
API calls before