Skip to content
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

FEATURE: Only load content tree nodes on demand #2927

Merged

Conversation

Sebobo
Copy link
Member

@Sebobo Sebobo commented Jul 23, 2021

Resolves: #2926

What I did

The content tree now behaves the same as the document tree. Only loading the nodes defined by loadingDepth initially and not relying purely on the nodedata rendered in the guestframe.
Toggling nodes with children also mimimcs the behaviour of the document tree and reloads the children when needed.

That reduces the number of initial db requests during page rendering in the backend from 119 to 78 in my test.

For large number of content nodes on a page this should improve performance considerably, especially if they are not all rendered.

How I did it

  • Removed the nonRenderedNodes rendering in the Neos.Neos:Page prototype
  • Adapt toggle/loading/updating mechanics from document tree to content tree
  • Load all necessary nodes defined by loadingDepth when reloading or switching documents and merge them with data from content (might make loading on toggle in some cases unnecessary later)

How to verify it

Work with content tree like before

Example video shows page with content that is not rendered, uncollapsing the parent node then loads the nodes (see loading spinner).
The other page renders all content, no further nodes need to be loaded.

Content.tree.node.loader.mov

@Sebobo Sebobo requested a review from dimaip July 23, 2021 06:42
@Sebobo Sebobo marked this pull request as ready for review July 23, 2021 09:40
@Sebobo
Copy link
Member Author

Sebobo commented Jul 23, 2021

Failing tests in master seems unrelated as it also fails for the other open PRs.

@grebaldi
Copy link
Contributor

Tests are green again :)

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Sebobo,

I've encountered an issue, though I have not identified a root cause yet.

So, I have a Content NodeType that has two auto-created content collections:

'Vendor.Site:Content.Section':
  superTypes:
    'Vendor.Site:Content': true
// ...
  childNodes:
    contents:
      type: Neos.Neos:ContentCollection
      constraints:
        nodeTypes:
          'Vendor.Site:Content': true
    hidden-contents:
      type: Neos.Neos:ContentCollection
      constraints:
        nodeTypes:
          'Vendor.Site:Content': true

The collection contents is being rendered with content element wrapping, while hidden-contents is not being rendered at all.

This leads to this behavior:

Before After
Screenshot_2021-07-25_15-00-52 https://user-images.githubusercontent.com/2522299/126901010-2d21160f-4c60-4113-842f-12c68083126a.mp4

I'll continue to investigate, but maybe you have an immediate idea what might be causing this :)

@Sebobo
Copy link
Member Author

Sebobo commented Jul 26, 2021

Hi @grebaldi thanks for testing.

Your example was one of the first issues I encountered after turning off the unrendered nodes rendering and had fixed. And with your nodetype definition and the following Fusion I'm not able to reproduce:

prototype(Vendor.Site:Content.Section) < prototype(Neos.Neos:ContentComponent) {
    renderer = afx`
        <div>
            <Neos.Neos:ContentCollection nodePath="contents" />
        </div>
    `
}

See in my video how it has part of the tree initially retrieved from the content and then it automatically loads the missing second collection:

two-child-collections.mov

So maybe anything special in your Fusion, that I did differently?

And can you check if the initial event for watchCurrentDocument is triggered for the content tree and executes a flowquery?

@Sebobo
Copy link
Member Author

Sebobo commented Jul 28, 2021

Fixed two bugs related to selecting content nodes that are not on the current page.

@grebaldi did you have another chance to verify your issue?
Maybe we should have a call and look at it together.

Copy link
Member

@markusguenther markusguenther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested the PR and did not see any issues like @grebaldi
So can not reproduce it. But maybe Wilhelm can check it again :)

The PR looks pretty clean. The change works like a charm on my machine ;)
Thanks to @Sebobo for this improvement 💚

Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still saw the issue and debugged a bit.

I could assert that the initial check for childrenAreFullyLoaded in the watchCurrentDocument saga wasn't performed recursively - that was the cause for the issue I've seen.

I added a suggestion that should fix it :)

packages/neos-ui-sagas/src/UI/ContentTree/index.js Outdated Show resolved Hide resolved
Co-authored-by: Wilhelm Behncke <2522299+grebaldi@users.noreply.github.com>
@Sebobo
Copy link
Member Author

Sebobo commented Jul 30, 2021

@grebaldi thx!

Can't see a difference, but makes sense :)
Commited the change.

@Sebobo Sebobo requested a review from grebaldi July 30, 2021 07:13
Copy link
Contributor

@grebaldi grebaldi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Sebobo,

I added another suggestion that should fix those eslint complaints that were introduced by my other suggestion. Sorry for that 😅

packages/neos-ui-sagas/src/UI/ContentTree/index.js Outdated Show resolved Hide resolved
Co-authored-by: Wilhelm Behncke <2522299+grebaldi@users.noreply.github.com>
@Sebobo Sebobo merged commit cb6847d into neos:master Aug 2, 2021
@Sebobo Sebobo deleted the feature/load-content-tree-nodes-from-backend branch August 2, 2021 08:46
Sebobo added a commit that referenced this pull request Nov 17, 2022
This way the content tree works even
when the guest frame crashes.
The information from the guest frame is not needed anymore since #2927.
This also make the UI more responsive as the content tree now loads at the same time as the document tree.

Resolves: #3251
Sebobo added a commit that referenced this pull request Nov 26, 2022
This way the content tree works even
when the guest frame crashes.
The information from the guest frame is not needed anymore since #2927 to build the content tree.
This also make the UI more responsive as the content tree now loads at the same time as the document tree.

It is still necessary sometimes to update the current document node when the guest frame is initialised as the actual rendered page might target a different document node than the one expected.

Resolves: #3251
markusguenther pushed a commit that referenced this pull request Nov 27, 2022
This way the content tree works even
when the guest frame crashes.
The information from the guest frame is not needed anymore since #2927 to build the content tree.
This also make the UI more responsive as the content tree now loads at the same time as the document tree.

It is still necessary sometimes to update the current document node when the guest frame is initialised as the actual rendered page might target a different document node than the one expected.

Resolves: #3251
skurfuerst added a commit to neos/neos-development-collection that referenced this pull request May 15, 2023
ahaeslich pushed a commit to queoGmbH/neos-development-collection that referenced this pull request May 16, 2023
neos-bot pushed a commit to neos/neos that referenced this pull request Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't load nodedata for structure tree from guest frame
3 participants