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

Performance issues on bigger forms #5111

Closed
jsardev opened this issue Jan 28, 2020 · 9 comments
Closed

Performance issues on bigger forms #5111

jsardev opened this issue Jan 28, 2020 · 9 comments
Labels
issue: bug Issue reporting a bug severity: medium If it breaks the basic use of the product but can be worked around source: core:admin Source is core/admin package status: confirmed Confirmed by a Strapi Team member or multiple community members

Comments

@jsardev
Copy link
Contributor

jsardev commented Jan 28, 2020

Describe the bug
I'm using Strapi as a CMS to control the content of my website. I wanted to be able to control every bigger element on my page via Strapi, so I've created this pretty flexible content-type (which is also pretty big). Unfortunately, when the Dynamic Zone field has already ~10 entries, the form becomes totally unusable (there's a massive lag on input).

Steps to reproduce the behavior

  1. Clone https://github.com/sarneeh/strapi-strapi-issues-5111
  2. Run Strapi npm i && npm run develop
  3. Go to Pages -> "Test" entry (the database is included)
  4. Try to type anything into a text field

Login credentials: test/test1234

Expected behavior
I expect the form to work fast.

Screenshots
strapi-issue

System

  • Node.js version: 12
  • NPM version: 6
  • Strapi version: beta.18.6
  • Database: SQLite
  • Operating system: MacOS
@derrickmehaffy
Copy link
Member

@sarneeh the fix has already been made but it isn't in any release yet (#5073). It will most likely be in beta.18.7 when that is released.

@jsardev
Copy link
Contributor Author

jsardev commented Jan 28, 2020

@sarneeh the fix has already been made but it isn't in any release yet (#5073). It will most likely be in beta.18.7 when that is released.

I've monkey-patched the fix and it is still sluggish (but slightly better).

Please let's leave this issue open until beta.18.7. I'll check this again on the next release. I'm almost certain that the issue is still there (but the root cause is different).

@derrickmehaffy
Copy link
Member

I do believe that @soupette did plan to make some changes into buffetjs as well so that may not be directly apparent until a release of buffet is made.

@soupette
Copy link
Contributor

Well the issue is big deeper. I have cloned the demo repo to see what happened and even with the fixes made in Buffet.js the issue still occurs. In the demo repo the form generated is quite big and React doesn't deal well with big DOM (list) so I'm thinking of adding react-virtualized to the edit view to solve the issue but the implementation will be quite complex...

@lauriejim lauriejim added severity: medium If it breaks the basic use of the product but can be worked around status: confirmed Confirmed by a Strapi Team member or multiple community members issue: bug Issue reporting a bug source: core:admin Source is core/admin package labels Feb 4, 2020
@wysher
Copy link
Contributor

wysher commented Feb 18, 2020

@soupette I belive that react-virtualized will NOT solve the problem. It will only COVER it until fields causing problems will be in view.

I've done ton of testing and found, that there are issues with some field types, which causes many rerenders onInput on any field. Problem applies especially to components containing time (time/datetime). In my tests I have only one text field + time field and already have performance issues.

So first thing to do is to review all components and fix perf. This should contains memoization to prevent unnecessary heavy computations (especially with arrays).

Next thing to do (without touching react-virtualized) is to remove collapsed elements from DOM. Repeatable components are great target for this. Collapsed component after collapsing animation should be removed, here's example code:

const [isOpen, setOpen] = useState(false);
const [isRendered, setRendered] = useState(false);

const toggle = () => {
  if (isOpen) {
    setOpen(false);
  } else {
    setRendered(true);
    setOpen(true);
  }
}

// or animationEnd, depends on how animation works 
handleTransitionEnd = ({ propertyName }) => {
  if (!isOpen && propertyName === 'height') {
    setRendered(false);
  }
}

return (
  <Component
    onClick={toggle}
    className={isOpen ? 'opened' : ''}
    onTransitionEnd={handleTransitionEnd}
  >
    {isRendered && <Content />}
  </Component>
)

I think only after fixing main performance issues you should consider using react-virtualized.

@soupette
Copy link
Contributor

@wysher thanks for taking the time to investigate it was my first idea see

But it doesn't really solve the issue and big big forms. So we will have to make other optimisations.
One could be solving the rerenders issue

@wysher
Copy link
Contributor

wysher commented Feb 18, 2020

@soupette I see.

Still there are issues with fields, because rerender onInput in one field causes rerenders in others, which shouldn't happen. That's what gif in first issue comment shows. And if this is the case, react-virtualized (react-window) will only cover existing problem.

Solving this unnecessary rerenders should fix this problem IMO. If not - we should find another way. As react team and many others suggest, only when other perfomance issues are fixed, virtualization should be considered, because virtualization has some impact in performance. Using virtualized with only fewer fields (most strapi use cases) will be an overkill.

@soupette
Copy link
Contributor

Well rerenders issue are caused by the context provider since it updates all the other inputs/elements updates are they are children of this provider. We will think of a way to improve this for the stable release but my idea is to use redux with some specific selectors...

But again the issue happens on really huge forms like the one provided in the issue.

@soupette
Copy link
Contributor

soupette commented Aug 7, 2020

I am closing this issue as it has been fixed with the rbac release. Feel free to reopen it.

@soupette soupette closed this as completed Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug severity: medium If it breaks the basic use of the product but can be worked around source: core:admin Source is core/admin package status: confirmed Confirmed by a Strapi Team member or multiple community members
Projects
None yet
Development

No branches or pull requests

5 participants