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

useBlockSync: Fix race condition when onChange callback changes #24012

Merged
merged 5 commits into from
Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions packages/block-editor/src/components/provider/test/use-block-sync.js
Original file line number Diff line number Diff line change
Expand Up @@ -362,4 +362,134 @@ describe( 'useBlockSync hook', () => {
);
expect( onInput ).not.toHaveBeenCalled();
} );

it( 'should use fresh callbacks if onChange/onInput have been updated when previous changes have been made', async () => {
const fakeBlocks = [
{ clientId: 'a', innerBlocks: [], attributes: { foo: 1 } },
];
const onChange1 = jest.fn();
const onInput = jest.fn();

let registry;
const setRegistry = ( reg ) => {
registry = reg;
};
let root;
await act( async () => {
root = create(
<TestWrapper
setRegistry={ setRegistry }
value={ fakeBlocks }
onChange={ onChange1 }
onInput={ onInput }
/>
);
} );

// Create a persistent change.
registry
.dispatch( 'core/block-editor' )
.updateBlockAttributes( 'a', { foo: 2 } );

const updatedBlocks1 = [
{ clientId: 'a', innerBlocks: [], attributes: { foo: 2 } },
];

expect( onChange1 ).toHaveBeenCalledWith( updatedBlocks1, {
selectionEnd: {},
selectionStart: {},
} );
noahtallen marked this conversation as resolved.
Show resolved Hide resolved

const newBlocks = [
{ clientId: 'b', innerBlocks: [], attributes: { foo: 1 } },
];

// Reset it so that we can test that it was not called after this point.
onChange1.mockReset();
const onChange2 = jest.fn();

// Update the component to point at a "different entity" (e.g. different
// blocks and onChange handler.)
await act( async () => {
root.update(
<TestWrapper
setRegistry={ setRegistry }
value={ newBlocks }
onChange={ onChange2 }
onInput={ onInput }
/>
);
} );

// Create a persistent change.
registry
.dispatch( 'core/block-editor' )
.updateBlockAttributes( 'b', { foo: 3 } );

// The first callback should not have been called.
expect( onChange1 ).not.toHaveBeenCalled();

// The second callback should be called with the new change.
expect( onChange2 ).toHaveBeenCalledWith(
[ { clientId: 'b', innerBlocks: [], attributes: { foo: 3 } } ],
{ selectionEnd: {}, selectionStart: {} }
);
} );

it( 'should use fresh callbacks if onChange/onInput have been updated when no previous changes have been made', async () => {
const fakeBlocks = [
{ clientId: 'a', innerBlocks: [], attributes: { foo: 1 } },
];
const onChange1 = jest.fn();
const onInput = jest.fn();

let registry;
const setRegistry = ( reg ) => {
registry = reg;
};
let root;
await act( async () => {
root = create(
<TestWrapper
setRegistry={ setRegistry }
value={ fakeBlocks }
onChange={ onChange1 }
onInput={ onInput }
/>
);
} );

const newBlocks = [
{ clientId: 'b', innerBlocks: [], attributes: { foo: 1 } },
];

const onChange2 = jest.fn();

// Update the component to point at a "different entity" (e.g. different
// blocks and onChange handler.)
await act( async () => {
root.update(
<TestWrapper
setRegistry={ setRegistry }
value={ newBlocks }
onChange={ onChange2 }
onInput={ onInput }
/>
);
} );

// Create a persistent change.
registry
.dispatch( 'core/block-editor' )
.updateBlockAttributes( 'b', { foo: 3 } );

// The first callback should never be called in this scenario.
expect( onChange1 ).not.toHaveBeenCalled();

// Only the new callback should be called.
expect( onChange2 ).toHaveBeenCalledWith(
[ { clientId: 'b', innerBlocks: [], attributes: { foo: 3 } } ],
{ selectionEnd: {}, selectionStart: {} }
);
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,9 @@ export default function useBlockSync( {

// Inform the controlling entity that changes have been made to
// the block-editor store they should be aware about.
const updateParent = isPersistent ? onChange : onInput;
const updateParent = isPersistent
? onChangeRef.current
: onInputRef.current;
updateParent( blocks, {
selectionStart: getSelectionStart(),
selectionEnd: getSelectionEnd(),
Expand Down