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.

Sign up for GitHub

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

Jump to bottom

Use flushSync to avoid render lag (fixes MBS-12424) #2559

Merged
merged 1 commit into from Jun 2, 2022
Merged

Use flushSync to avoid render lag (fixes MBS-12424) #2559

merged 1 commit into from Jun 2, 2022

Conversation

mwiencek
Copy link
Member

@mwiencek mwiencek commented Jun 2, 2022

React v18 uses a new method of rendering that can be interrupted and doesn't block the page from performing other work. This can make updates appear laggy where we're trying to render many successive roots -- there is a noticeable delay between rendering each root where React yields its time. (In the case of MBS-12424, each "root" would be a track artist credit container.) This isn't really a problem with React, but a side effect of how we're gluing it into non-React code. Normally we wouldn't have so many roots.

In order to make our haphazard way of rendering things into non-React pages appear more snappy, I'm wrapping all calls to render and hydrateRoot with flushSync, which causes all pending updates to the DOM to occur immediately. This keeps that behavior in line with how React v17 worked.

Wrapping hydrateRoot in particular may affect the timing of when user scripts are allowed to interact with the page and avoid hydration errors.

@mwiencek mwiencek mentioned this pull request Jun 2, 2022
Closed
@reosarevok
Copy link
Member

Is this intended to be permanent, or to be removed once we don't use React embeds in TT files anymore?

https://reactjs.org/docs/react-dom.html#flushsync says "flushSync can significantly hurt performance. Use sparingly." so it makes it sound less than ideal.

@mwiencek
Copy link
Member Author

mwiencek commented Jun 2, 2022

Is this intended to be permanent, or to be removed once we don't use React embeds in TT files anymore?

I'd suggest permanent for the hydrateRoot call, to keep the timing of hydration consistent for user scripts. The ones around the render calls wouldn't be permanent, because eventually those would go away and just become part of a normal React component tree (with only one root).

https://reactjs.org/docs/react-dom.html#flushsync says "flushSync can significantly hurt performance. Use sparingly." so it makes it sound less than ideal.

It defeats the purpose of the concurrency features they developed for React 18. It'd be an issue if within a normal component we were wrapping a bunch of setState or dispatch calls in flushSync, since that's not letting React batch updates together or interrupt rendering if needed. For render calls, this just reverts back to the React v17 behavior, and it's not an area we can use concurrency at all (since we're mixing it with knockout).

reosarevok
reosarevok approved these changes Jun 2, 2022
View reviewed changes
Copy link
Member

@reosarevok reosarevok left a comment

Choose a reason for hiding this comment

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

Ok, in that case, happy to make the change, but should we push it to beta and release it today to avoid the laggy thing appearing in prod?

@mwiencek
Use flushSync to avoid render lag (fixes MBS-12424)
db4ad19 
React v18 uses a new method of rendering that can be interrupted and
doesn't block the page from performing other work. This can make updates
appear laggy where we're trying to render many successive roots -- there
is a noticeable delay between rendering each root where React yields its
time. (In the case of MBS-12424, each "root" would be a track artist
credit container.) This isn't really a problem with React, but a side
effect of how we're gluing it into non-React code. Normally we wouldn't
have so many roots.

In order to make our haphazard way of rendering things into non-React
pages appear more snappy, I'm wrapping all calls to `render` and
`hydrateRoot` with `flushSync`, which causes all pending updates to the
DOM to occur immediately. This keeps that behavior in line with how
React v17 worked.

Wrapping `hydrateRoot` in particular may affect the timing of when user
scripts are allowed to interact with the page and avoid hydration
errors.
@mwiencek mwiencek changed the base branch from master to beta June 2, 2022 15:28
@mwiencek
Copy link
Member Author

mwiencek commented Jun 2, 2022

Oops, that was my intent. Fixed the PR to target beta instead.

@mwiencek mwiencek added the Regression/Beta Bugs that are either on beta or new regressions and should be checked ASAP label Jun 2, 2022
@mwiencek mwiencek merged commit 62e78c0 into metabrainz:beta Jun 2, 2022
@mwiencek mwiencek deleted the react-18-flush-sync branch June 2, 2022 16:48
mwiencek added a commit that referenced this pull request Jun 2, 2022
@mwiencek
Merge branch 'beta'
d63f32f 
* beta:
  Use flushSync to avoid render lag (fixes MBS-12424) (#2559)
  Update translations from Transifex
  MBS-12420: Support comma-separated IDs for edit search types again (#2556)
  MBS-12416: Avoid wrapping in menu links
  MBS-12415: Stop most interface wrapping
mwiencek added a commit that referenced this pull request Jun 2, 2022
@mwiencek
Merge branch 'master' into homepage-revamp
b6ee53d 
* master:
  Use flushSync to avoid render lag (fixes MBS-12424) (#2559)
  Update translations from Transifex
  MBS-12420: Support comma-separated IDs for edit search types again (#2556)
  MBS-12416: Avoid wrapping in menu links
  MBS-12415: Stop most interface wrapping
  Update POT files using the production database
  Update translations from Transifex
  MBS-12328: Add #content to entity/Edits for artwork display (#2502)
  MBS-12336: Validate that Predicate::Set is being passed integers (#2509)
  MBS-12333: Block smart links: share.amuse.io (#2501)
  Run all sql before enabling replication triggers (#2539)
  MBS-12412: Fail gracefully if requesting an invalid ID in edit-cover-art (#2547)
  MBS-12387: Add titles to URL editor edit buttons (#2550)
  Bump react-table to 7.8.0 (#2551)
  MBS-12407: Add tobarandualchais.co.uk to the other dbs whitelist (#2545)
  MBS-12280 (II): Add target _blank to more edit form links (#2546)
  MBS-12413: Use uri_escape_utf8 on Data::Wikidoc (#2548)
  ExportAllTables.t: remove --nodbmirror2 flags (#2538)
  MBS-12398: Update release_meta.amazon_asin (#2543)
  Bump Flow to 0.179.0
  JSON dumps: ignore more useless primary/foreign keys (#2536)
  MBS-12404: Replace Text::Unaccent (#2542)
  MBS-12353: Actually check proposed ratings are allowed (#2511)
  MBS-8193 / MBS-12332: Wrap absurdly long lines (#2516)
  MBS-11694: Cleanup /intent/user Twitter URLs (#2519)
  MBS-8875: Improve CatNoLooksLikeASIN regexp (#2520)
  MBS-12347: Also show RG artist in autocomplete if type is null (#2517)
  MBS-12385: Add report for digital releases with mail order rels (#2535)
  MBS-12212: Add tests for event filtering
  MBS-12212: Allow filtering event list by setlist contents
  MBS-12212: Add filtering to artists' events tab
  Update react, react-dom to v18 (#2544)
  Bump Flow to 0.178.1
  Upgrade Flow to 0.178.0
  Fix spammy PG warnings in ProcessReplicationChanges
  Remove BundleReplicationPackets
  MBS-4960: Give useful error when checking donation status fails
  MBS-12361: Allow navigation from donation check page
reosarevok added a commit that referenced this pull request Jun 7, 2022
@reosarevok
Merge branch 'beta' into production
923237c 
* beta:
  Update POT files using the production database
  Update translations from Transifex
  MBS-12437: Limit overflow-wrap even more (#2561)
  Use flushSync to avoid render lag (fixes MBS-12424) (#2559)
  Update translations from Transifex
  MBS-12420: Support comma-separated IDs for edit search types again (#2556)
  MBS-12416: Avoid wrapping in menu links
  MBS-12415: Stop most interface wrapping
  Update POT files using the production database
  Update translations from Transifex
  MBS-12328: Add #content to entity/Edits for artwork display (#2502)
  MBS-12336: Validate that Predicate::Set is being passed integers (#2509)
  MBS-12333: Block smart links: share.amuse.io (#2501)
  Run all sql before enabling replication triggers (#2539)
  MBS-12412: Fail gracefully if requesting an invalid ID in edit-cover-art (#2547)
  MBS-12387: Add titles to URL editor edit buttons (#2550)
  Bump react-table to 7.8.0 (#2551)
  MBS-12407: Add tobarandualchais.co.uk to the other dbs whitelist (#2545)
  MBS-12280 (II): Add target _blank to more edit form links (#2546)
  MBS-12413: Use uri_escape_utf8 on Data::Wikidoc (#2548)
  ExportAllTables.t: remove --nodbmirror2 flags (#2538)
  MBS-12398: Update release_meta.amazon_asin (#2543)
  Bump Flow to 0.179.0
  JSON dumps: ignore more useless primary/foreign keys (#2536)
  MBS-12404: Replace Text::Unaccent (#2542)
  MBS-12353: Actually check proposed ratings are allowed (#2511)
  MBS-8193 / MBS-12332: Wrap absurdly long lines (#2516)
  MBS-11694: Cleanup /intent/user Twitter URLs (#2519)
  MBS-8875: Improve CatNoLooksLikeASIN regexp (#2520)
  MBS-12347: Also show RG artist in autocomplete if type is null (#2517)
  MBS-12385: Add report for digital releases with mail order rels (#2535)
  MBS-12212: Add tests for event filtering
  MBS-12212: Allow filtering event list by setlist contents
  MBS-12212: Add filtering to artists' events tab
  Update react, react-dom to v18 (#2544)
  Bump Flow to 0.178.1
  Upgrade Flow to 0.178.0
  Fix spammy PG warnings in ProcessReplicationChanges
  Remove BundleReplicationPackets
  MBS-4960: Give useful error when checking donation status fails
  MBS-12361: Allow navigation from donation check page
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reosarevok reosarevok reosarevok approved these changes

Assignees
No one assigned
Labels
Regression/Beta Bugs that are either on beta or new regressions and should be checked ASAP
Projects
None yet
Milestone
No milestone
2 participants
@mwiencek @reosarevok