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

Conditional useSelect not working as expected #47145

Closed
davilera opened this issue Jan 13, 2023 · 6 comments · Fixed by #47243
Closed

Conditional useSelect not working as expected #47145

davilera opened this issue Jan 13, 2023 · 6 comments · Fixed by #47243
Assignees
Labels
[Package] Data /packages/data [Type] Bug An existing feature does not function as intended

Comments

@davilera
Copy link
Contributor

Description

useSelect doesn't re-render a component after the store is updated if useSelect's callback uses the select argument conditionally. In other words...

This hook doesn't work:

useValue = ( condition ) => useSelect( ( select ) =>
  !! condition ? select( STORE ).getValue() : ''
);

but this one does:

useValue = ( condition ) => useSelect( ( select ) => {
  const { getValue } = select( STORE );
  return !! condition ? getValue() : '';
} );

Step-by-step reproduction instructions

  1. Open Gutenberg
  2. Open your browser's JS console
  3. Register a simple store to keep track of a single string:
STORE_NAME = 'david/test';
wp.data.register(
  wp.data.createReduxStore( STORE_NAME, {
    reducer: ( state = '', action ) =>
      action.type === 'SET_VALUE' ? action.value : state,
    actions: {
      setValue: ( value ) => ( { type: 'SET_VALUE', value } ),
    },
    selectors: {
      getValue: ( state ) => state,
    },
  } )
);
  1. Create a simple component to test the useSelect logic:
Main = () => {
  const { useEffect, useState, createElement: el } = wp.element;
  const { useSelect, useDispatch } = wp.data;

  const [ text, setText ] = useState( '' );
  const link = ( ` ${ text } `.match( / https?:\/\/[^ ]+ / ) || [ '' ] )[0].trim();

  // Conditional read from store
  const value = useSelect( ( select ) =>
    !! link ? select( STORE_NAME ).getValue() : ''
  );

  // Write to store
  const { setValue } = useDispatch( STORE_NAME );
  useEffect( () => {
    setValue( link );
  }, [ link ] );
  
  const { TextControl } = wp.components;
  return el( 'div', {}, [
    el( TextControl, { key: 'text-control', value: text, onChange: setText } ),
    el( 'pre', { key: 'pre' }, JSON.stringify( value ) ),
  ] );
}

(This component lets you write some text. It extracts the first link and saves it in the store with a useEffect).

  1. Render the component:
document.body.innerHTML = '<div id="root"></div>';
wp.element.render( wp.element.createElement( Main ), document.getElementById( 'root' ) );
  1. Copy and paste the following text in the TextControl: Hello, World. https://example.com

Expected behavior: You should see "https://example.com" in the pre tag
Actual behavior: You see the empty string "", even though wp.data.select( STORE_NAME ).getValue() returns the proper value https://example.com.

Screenshots, screen recording, code snippet

No response

Environment info

  • WordPress 6.1.1 without Gutenberg

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@TimothyBJacobs TimothyBJacobs added the [Package] Data /packages/data label Jan 15, 2023
@TimothyBJacobs
Copy link
Member

The useSelect hook accepts a dependencies array as it's second parameter like other React hooks. Your link value should be passed to this dependency array, which I think should fix your issue.

const value = useSelect( ( select ) =>
    !! link ? select( STORE_NAME ).getValue() : '',
    [ link ]
  );

@davilera
Copy link
Contributor Author

Thanks for the tip, @TimothyBJacobs. It looks like, indeed, the dependency array fixes the issue.

I was just surprised that this hook doesn't work:

useValue = ( condition ) => useSelect( ( select ) =>
  !! condition ? select( STORE ).getValue() : ''
);

but this one does:

useValue = ( condition ) => useSelect( ( select ) => {
  const { getValue } = select( STORE );
  return !! condition ? getValue() : '';
} );

as if the mere act of calling the select once did some magic behind the scenes.

If that were the case, I think it'd be a good idea if we could explicitly mention this in the documentation. What do you think?

@youknowriad
Copy link
Contributor

Related to the discussion we had in the useSelect refactoring PR cc @jsnajdr @kevin940726

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Jan 16, 2023
@youknowriad
Copy link
Contributor

I agree that we should either fix this as a bug or mention in the documentation that conditional selects are not supported.

@jsnajdr
Copy link
Member

jsnajdr commented Jan 16, 2023

Let's fix this as a bug -- it shouldn't be terribly hard or expensive to compare the list of selected stores after each call. Especially with the new useSyncExternalStore implementation.

@jsnajdr
Copy link
Member

jsnajdr commented Jan 18, 2023

Proposing a fix in #47243.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants