Skip to content

Commit

Permalink
useBlockSync: Fix race condition when onChange callback changes (#24012)
Browse files Browse the repository at this point in the history
* Fix race condition when onChange callback changed

* Add tests to cover the scenario
  • Loading branch information
noahtallen authored Jul 20, 2020
1 parent e0d6243 commit a8e49fc
Show file tree
Hide file tree
Showing 2 changed files with 133 additions and 1 deletion.
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: {},
} );

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

0 comments on commit a8e49fc

Please sign in to comment.