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

Jump to bottom

Fix useBlockSync race condition #23292

Merged
merged 2 commits into from Jun 20, 2020
Merged

Fix useBlockSync race condition #23292

merged 2 commits into from Jun 20, 2020

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jun 18, 2020

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 Jun 18, 2020
@youknowriad youknowriad self-assigned this Jun 18, 2020
@youknowriad youknowriad added [Package] Block editor [Type] Bug labels Jun 18, 2020
@github-actions
Copy link

@github-actions github-actions bot commented Jun 18, 2020 •
edited

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

epiqueras
epiqueras requested changes Jun 18, 2020
View changes
packages/block-editor/src/components/provider/use-block-sync.js Outdated
@@ -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

@epiqueras epiqueras Jun 18, 2020

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

@youknowriad youknowriad Jun 18, 2020

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

@epiqueras epiqueras Jun 19, 2020

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

@youknowriad youknowriad Jun 20, 2020

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 Jun 20, 2020
@youknowriad youknowriad merged commit aaa0b32 into master Jun 20, 2020
4 checks passed
@youknowriad youknowriad deleted the fix/race-condition-use-block-sync branch Jun 20, 2020
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 20, 2020
@noisysocks noisysocks mentioned this pull request Jun 24, 2020
Closed
@noisysocks noisysocks mentioned this pull request Jun 24, 2020
Merged
@noahtallen noahtallen mentioned this pull request Jul 17, 2020
Merged
6 tasks
@mkaz mkaz mentioned this pull request Jan 27, 2021
Closed
37 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noahtallen noahtallen

@epiqueras epiqueras

@ellatrix ellatrix

Assignees

@youknowriad youknowriad

Labels
[Package] Block editor [Type] Bug
Projects
None yet
Milestone
Gutenberg 8.4
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@youknowriad @noahtallen @epiqueras