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

Fix useBlockSync race condition #23292

Merged
merged 2 commits into from
Jun 20, 2020
Merged

Conversation

youknowriad
Copy link
Contributor

I noticed this while working on https://github.com/youknowriad/asblocks.

Sometimes when you change something in blocks, like type a character, the onChange handler gets called twice for the same value, causing all sorts of issues if a new change happened in the meantime. I think it's related to the fact that the subscription is "renewed" each time onChange or onInput changes. This shouldn't be the case and subscriptions should just use the last callbacks. That's what this PR does (that solved the issue for me btw)

@youknowriad youknowriad requested a review from ellatrix as a code owner June 18, 2020 22:39
@youknowriad youknowriad self-assigned this Jun 18, 2020
@youknowriad youknowriad added [Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended labels Jun 18, 2020
@github-actions
Copy link

github-actions bot commented Jun 18, 2020

Size Change: -11 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-editor/index.js 107 kB -11 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 7.26 kB 0 B
build/block-directory/style-rtl.css 937 B 0 B
build/block-directory/style.css 937 B 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.87 kB 0 B
build/block-library/editor.css 7.87 kB 0 B
build/block-library/index.js 129 kB 0 B
build/block-library/style-rtl.css 8 kB 0 B
build/block-library/style.css 8.01 kB 0 B
build/block-library/theme-rtl.css 730 B 0 B
build/block-library/theme.css 732 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/blocks/index.js 48.1 kB 0 B
build/components/index.js 196 kB 0 B
build/components/style-rtl.css 15.9 kB 0 B
build/components/style.css 15.8 kB 0 B
build/compose/index.js 9.6 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.17 kB 0 B
build/edit-navigation/index.js 8.26 kB 0 B
build/edit-navigation/style-rtl.css 1.02 kB 0 B
build/edit-navigation/style.css 1.02 kB 0 B
build/edit-post/index.js 303 kB 0 B
build/edit-post/style-rtl.css 5.5 kB 0 B
build/edit-post/style.css 5.5 kB 0 B
build/edit-site/index.js 16.6 kB 0 B
build/edit-site/style-rtl.css 3.03 kB 0 B
build/edit-site/style.css 3.03 kB 0 B
build/edit-widgets/index.js 9.33 kB 0 B
build/edit-widgets/style-rtl.css 2.43 kB 0 B
build/edit-widgets/style.css 2.43 kB 0 B
build/editor/editor-styles-rtl.css 468 B 0 B
build/editor/editor-styles.css 469 B 0 B
build/editor/index.js 44.7 kB 0 B
build/editor/style-rtl.css 3.81 kB 0 B
build/editor/style.css 3.81 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 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.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 450 B 0 B
build/list-reusable-blocks/style.css 451 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 663 B 0 B
build/nux/style.css 660 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.85 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@@ -108,6 +108,9 @@ export default function useBlockSync( {
// waiting for React renders for changes.
const onInputRef = useRef( onInput );
const onChangeRef = useRef( onChange );
onInputRef.current = onInput;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a side effect, so it needs to be wrapped in an effect hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrapped in an effect but I suspect that it's what's causing the issue. Subscriptions might be called with an outdated listener before the effect runs, maybe I should use useLayoutEffect. I'll need to confirm again in asblocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, useLayoutEffect is used for stuff like this that needs to be sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tested again, and useEffect seems sufficient so i think we can move forward here.

@youknowriad youknowriad force-pushed the fix/race-condition-use-block-sync branch from baafa5b to 4dfdee4 Compare June 20, 2020 09:00
@youknowriad youknowriad merged commit aaa0b32 into master Jun 20, 2020
@youknowriad youknowriad deleted the fix/race-condition-use-block-sync branch June 20, 2020 16:53
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 20, 2020
This was referenced Jun 24, 2020
@mkaz mkaz mentioned this pull request Jan 27, 2021
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Block editor /packages/block-editor [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants