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

Throttle useSelect calls #26695

Closed
wants to merge 4 commits into from
Closed

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented Nov 4, 2020

Description

I added a console.log statement to onStoreChange(); and opened the post editor. Then, I typed a single letter:

Zrzut ekranu 2020-11-4 o 16 10 41

Uh-oh!

Then I did the same with this PR applied:

Zrzut ekranu 2020-11-4 o 16 15 30

Still not perfect, but much better!

How has this been tested?

Confirm all the tests pass

Types of changes

Bugfix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@adamziel adamziel added the [Package] Core data /packages/core-data label Nov 4, 2020
@adamziel adamziel requested a review from nerrad as a code owner November 4, 2020 15:17
@adamziel adamziel self-assigned this Nov 4, 2020
@adamziel adamziel mentioned this pull request Nov 4, 2020
6 tasks
@adamziel
Copy link
Contributor Author

adamziel commented Nov 4, 2020

The timing changes caused some tests to fail. Let's discuss this idea and I'll look into fixing the tests if it gets any traction.

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

Size Change: +528 B (0%)

Total Size: 1.21 MB

Filename Size Change
build/block-editor/index.js 131 kB +388 B (0%)
build/block-library/index.js 147 kB +23 B (0%)
build/blocks/index.js 48.1 kB +11 B (0%)
build/data/index.js 8.82 kB +50 B (0%)
build/edit-site/index.js 22.4 kB +56 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 9.06 kB 0 B
build/block-library/editor.css 9.06 kB 0 B
build/block-library/style-rtl.css 7.9 kB 0 B
build/block-library/style.css 7.9 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.5 kB 0 B
build/data-controls/index.js 772 B 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.41 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/index.js 26.3 kB 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.16 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@ellatrix
Copy link
Member

ellatrix commented Nov 4, 2020

Cc @youknowriad. Most selectors already run async, except for the selected block's. I don't think we should delay it more than that.

@adamziel
Copy link
Contributor Author

adamziel commented Nov 5, 2020

@ellatrix you mean the priority queue? How about only executing each selector once even if it's enqueued multiple times?

@gziolo
Copy link
Member

gziolo commented Nov 5, 2020

See how AsyncModeProvider component is integrated:

<AsyncModeProvider
key={ clientId }
value={ ! isBlockInSelection }
>

Only components down the tree of the currently selected block list item are processed in real-time, everything else is updated asynchronously, and only when there are free cycles left to process those datastore updates.

The number of failing e2e tests is so huge that it should be a good indicator that it has a negative impact on the writing flow.

In general, throttling has always been problematic when applied inside the editor.

@adamziel
Copy link
Contributor Author

adamziel commented Nov 5, 2020

Makes sense, that's helpful feedback. I'll close this PR then.

@adamziel adamziel closed this Nov 5, 2020
@gziolo gziolo deleted the try/throttle-use-select-calls branch November 5, 2020 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Core data /packages/core-data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants