Skip to content

Commit

Permalink
Capture state changes scheduled between render and effect (#38509)
Browse files Browse the repository at this point in the history
* Add failing useSelect test case when state updates before subscription

In the event that a child component dispatched an action modifying the
store, the changes in the store were lost due to `useSelect`'s
`useIsomorphicLayoutEffect` referencing a stale value.

* Capture state changes before useSelect subscription

In the event that a child component dispatched an action modifying the
store, the changes in the store were lost due to `useSelect`'s
`useIsomorphicLayoutEffect` referencing a stale value.

This change ensures the `useIsomorphicLayoutEffect` references the
latest `mapOutput` when setting the related `latestMapOutput.current`
value.

* Revert native editor workaround for stale useSelect value

Store updates triggered from within the post title component's
`componentDidUpdate` hook resulted in stale `isTitledSelected` values
returned from the memoized `useSelect`. This reverts a workaround that
avoided memoization by splitting the `useSelect` hook usage in two.

* refactor: Simplify and improve useSelect test

* refactor: Delay mapping selector output until selector is called

This avoids stale mappings overwriting new values in specific contexts
where `onStoreChange` updates the value before the effect writes the
original mapping.

* fix: Rename test description

This test focuses upon the order in which state is read and written. It
is not related to when the subscription is set.

* refactor: Group related selector output mapping logic

Reduce cognitive load of code by grouping related logic.
  • Loading branch information
dcalhoun authored Sep 16, 2022
1 parent e0464ed commit 533e0b7
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 20 deletions.
12 changes: 10 additions & 2 deletions packages/data/src/components/use-select/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export default function useSelect( mapSelect, deps ) {

let mapOutput;

let selectorRan = false;
if ( _mapSelect ) {
mapOutput = latestMapOutput.current;
const hasReplacedRegistry = latestRegistry.current !== registry;
Expand All @@ -168,6 +169,7 @@ export default function useSelect( mapSelect, deps ) {
) {
try {
mapOutput = wrapSelect( _mapSelect );
selectorRan = true;
} catch ( error ) {
let errorMessage = `An error occurred while running 'mapSelect': ${ error.message }`;

Expand All @@ -191,7 +193,9 @@ export default function useSelect( mapSelect, deps ) {
latestRegistry.current = registry;
latestMapSelect.current = _mapSelect;
latestIsAsync.current = isAsync;
latestMapOutput.current = mapOutput;
if ( selectorRan ) {
latestMapOutput.current = mapOutput;
}
latestMapOutputError.current = undefined;
} );

Expand Down Expand Up @@ -310,9 +314,11 @@ export function useSuspenseSelect( mapSelect, deps ) {
const hasReplacedMapSelect = latestMapSelect.current !== _mapSelect;
const hasLeftAsyncMode = latestIsAsync.current && ! isAsync;

let selectorRan = false;
if ( hasReplacedRegistry || hasReplacedMapSelect || hasLeftAsyncMode ) {
try {
mapOutput = wrapSelect( _mapSelect );
selectorRan = true;
} catch ( error ) {
mapOutputError = error;
}
Expand All @@ -322,7 +328,9 @@ export function useSuspenseSelect( mapSelect, deps ) {
latestRegistry.current = registry;
latestMapSelect.current = _mapSelect;
latestIsAsync.current = isAsync;
latestMapOutput.current = mapOutput;
if ( selectorRan ) {
latestMapOutput.current = mapOutput;
}
latestMapOutputError.current = mapOutputError;
} );

Expand Down
61 changes: 59 additions & 2 deletions packages/data/src/components/use-select/test/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/**
* External dependencies
*/
import { act, render } from '@testing-library/react';
import { act, render, fireEvent } from '@testing-library/react';

/**
* WordPress dependencies
*/
import { useState, useReducer } from '@wordpress/element';
import { Component, useState, useReducer } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -434,6 +434,63 @@ describe( 'useSelect', () => {
);
} );

it( 'captures state changes scheduled between render and effect', () => {
registry.registerStore( 'store-1', counterStore );

class ChildComponent extends Component {
componentDidUpdate( prevProps ) {
if (
this.props.childShouldDispatch &&
this.props.childShouldDispatch !==
prevProps.childShouldDispatch
) {
registry.dispatch( 'store-1' ).increment();
}
}

render() {
return null;
}
}

const selectCount1AndDep = jest.fn( ( select ) => ( {
count1: select( 'store-1' ).getCounter(),
} ) );

const TestComponent = () => {
const [ childShouldDispatch, setChildShouldDispatch ] =
useState( false );
const state = useSelect( selectCount1AndDep, [] );

return (
<>
<div role="status">count1:{ state.count1 }</div>
<ChildComponent
childShouldDispatch={ childShouldDispatch }
/>
<button
onClick={ () => setChildShouldDispatch( true ) }
>
triggerChildDispatch
</button>
</>
);
};

const rendered = render(
<RegistryProvider value={ registry }>
<TestComponent />
</RegistryProvider>
);

fireEvent.click( rendered.getByText( 'triggerChildDispatch' ) );

expect( selectCount1AndDep ).toHaveBeenCalledTimes( 3 );
expect( rendered.getByRole( 'status' ) ).toHaveTextContent(
'count1:1'
);
} );

it( 'handles registry selectors', () => {
const getCount1And2 = createRegistrySelector(
( select ) => ( state ) => ( {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,25 +16,24 @@ function useNativeBlockEditorSettings( settings, hasTemplate ) {
const editorSettings = useBlockEditorSettings( settings, hasTemplate );

const supportReusableBlock = capabilities.reusableBlock === true;
const { reusableBlocks } = useSelect(
( select ) => ( {
reusableBlocks: supportReusableBlock
? select( coreStore ).getEntityRecords(
'postType',
'wp_block',
// Unbounded queries are not supported on native so as a workaround, we set per_page with the maximum value that native version can handle.
// Related issue: https://github.com/wordpress-mobile/gutenberg-mobile/issues/2661
{ per_page: 100 }
)
: [],
} ),
const { reusableBlocks, isTitleSelected } = useSelect(
( select ) => {
return {
reusableBlocks: supportReusableBlock
? select( coreStore ).getEntityRecords(
'postType',
'wp_block',
// Unbounded queries are not supported on native so as a workaround, we set per_page with the maximum value that native version can handle.
// Related issue: https://github.com/wordpress-mobile/gutenberg-mobile/issues/2661
{ per_page: 100 }
)
: [],
isTitleSelected: select( editorStore ).isPostTitleSelected(),
};
},
[ supportReusableBlock ]
);

const { isTitleSelected } = useSelect( ( select ) => ( {
isTitleSelected: select( editorStore ).isPostTitleSelected(),
} ) );

return useMemo(
() => ( {
...editorSettings,
Expand Down

0 comments on commit 533e0b7

Please sign in to comment.