plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
454 stars 613 forks source link

Don't request sub items by default #4386

Open wesleybl opened 1 year ago

wesleybl commented 1 year ago

I think at the beginning of Volto/Restapi, most of the content was not containers. Now that everything is container, I think the content request shouldn't request the subitems by default. Who defines if the subitems are necessary, are the blocks, and not the standard request of the content. I think this would improve performance, since we wouldn't have "repeated" subitems in requests.

tiberiuichim commented 1 year ago

@wesleybl it's not clear to me what you mean. Could you point to source code, or maybe explain what you mean by "content request shouldn't request the subitems by default" ?

wesleybl commented 1 year ago

@tiberiuichim when we make a request for:

http://localhost:3000/doc

Volto makes a request to:

http://localhost:3000/++api++/doc

This request returns a json, with a key items. This key contains the subitems of doc. In doc view, these subitems are not used unless a block needs them. But most blocks that use subitems make another request. So it is not necessary, by default, these subitems to be in the first request.

tiberiuichim commented 1 year ago

@wesleybl

I agree with the sentiment, but I think the problem is more complex and by removing those we may be causing problems.

While it's true that potentially any block that lists items would fetch them through a querysearch, that's not a rule that applies to content type views. For example, we have a content type called "Publication" that uses its children to create an album view.

Btw, this problem is not really in Volto's domain, it would need to be solved in plone.restapi

wesleybl commented 1 year ago

@tiberiuichim so is there already a way to make a request to plone.restapi, telling it not to return the subitems?

Could we then have a "No subitems" setting in Volto with a default value False?

davisagli commented 1 year ago

@wesleybl I took a look at the code; the plone.restapi content serializer already supports skipping the subitems by setting include_items to a falsy value in the request query params: https://github.com/plone/plone.restapi/blob/master/src/plone/restapi/serializer/dxcontent.py#L164

So maybe it is possible to customize Volto's use of the API to send that value. But it's a bit of a tricky question how volto can know when they are needed and when they aren't.

The concept of sometimes including items and sometimes omitting them feels similar to plone.restapi's concept of API expanders. I wonder if it should be refactored into an items expander so it can be controlled in the same way as other expanders like breadcrumbs, etc. But that wouldn't really solve the tricky question...

wesleybl commented 1 year ago

But it's a bit of a tricky question how volto can know when they are needed and when they aren't.

Yes this is really complicated. If I understood correctly, the rendering of the blocks is done after the content is requested. so I don't know if the block could "warn" that it needs the subitems. Could it be an extra request, only with the subitems?

tiberiuichim commented 1 year ago

@wesleybl I'm probably reiterating things you already know, but for anyone else watching this conversation, the data flow is like this:

So, if you need the children serialized in the main request, it has to happen based on decisions that happen in the backend. Now, if you completely omit the children from being serialized, you could write a @component that gets automatically "expanded" when you find a certain block in the serialized context data.

wesleybl commented 1 year ago

@tiberiuichim I don't know if the use of @components would be ideal, because it would bring the object's data again, and in this second moment, the ideal would be to bring only the subitems. Maybe a simple search would solve it.

It might be possible to do this in a subitemsNeed wrapper, which checks to see if the subitems are filled in and fills in if not. So this would be transparent for the blocks that use the subitems. It would only be necessary to change from:

export default MyBlock

to:

export default subitemsNeed(MyBlock)

But we would still have the setting to not enable this by default so as not to break things.

tiberiuichim commented 1 year ago

@wesleybl Here's a recipe on what you need to do:

wesleybl commented 1 year ago

@tiberiuichim I understand that your suggestion would be specific to me. But I would like to have something in the core, so I don't have to customize things in each project.

tiberiuichim commented 1 year ago

@wesleybl I think we need a @plone/volto-team decision on this one. It would be a heavy breaking change, though, as there are websites that may depend on having the children summary serialized with the main request.

wesleybl commented 1 year ago

@tiberiuichim as i said in https://github.com/plone/volto/issues/4386#issuecomment-1433040216, we could have a configuration in Volto remove the children. Thus, the change would only occur if the project explicitly changed this configuration.

tiberiuichim commented 1 year ago

@wesleybl Ok, so the proposal is that Volto, by default, passes include_items=False to the getContent action.

I think that would only half solve the problem. In theory I agree with you: having the children serialized by default seems nasty, but I only have two reasons for this:

But the problem that we're unearthing by excluding the children are a lot greater for the content types that need them. As I said, if your content type needs the children, you can't solve this problem elegantly in pure Volto, because Volto knows about the content type too late, as the getContent was already executed for the context.

Sorry if I'm missing something, I hope not.

wesleybl commented 1 year ago

@tiberiuichim as i said in https://github.com/plone/volto/issues/4386#issuecomment-1458383937, we could have a wrapper that "completes the subitems" of the content if it finds a block that needs them.