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

Components: Use useStoreState() instead of store.useState() #64648

Merged
merged 3 commits into from
Aug 21, 2024

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Aug 20, 2024

What?

After updating to Ariakit v0.4.10, we're changing store.useState() instances to the new useStoreState() API.

Why?

To resolve problems with the React Compiler raising errors when using store.useState():

Hooks must be the same function on every render, but this value may change over time to a different function. See https://react.dev/reference/rules/react-calls-components-and-hooks#dont-dynamically-use-hooks  react-compiler/react-compiler

See #61788.

How?

We're updating the syntax.
For instances that were conditionally called, we're moving them above the condition as needed.

Testing Instructions

  • Test the affected components in Storybook and verify they still work well.
  • Verify all checks are green.

Testing Instructions for Keyboard

None

Screenshots or screencast

None

@tyxla tyxla added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Aug 20, 2024
@tyxla tyxla requested review from Mamaduka and a team August 20, 2024 14:53
@tyxla tyxla self-assigned this Aug 20, 2024
@tyxla tyxla requested a review from ajitbohra as a code owner August 20, 2024 14:53
Copy link

github-actions bot commented Aug 20, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tyxla <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: ciampo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

packages/components/src/tabs/tablist.tsx Outdated Show resolved Hide resolved
if ( ! context ) {
warning( '`Tabs.TabPanel` must be wrapped in a `Tabs` component.' );
return null;
}
const { store, instanceId } = context;
const instancedTabId = `${ instanceId }-${ tabId }`;
const selectedId = store.useState( ( state ) => state.selectedId );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved up above the condition to fulfill the rules of hooks. Simplified the callback syntax.

@@ -359,10 +361,11 @@ export default function ViewList< Item >( props: ViewListProps< Item > ) {

const store = useCompositeStore( {
defaultActiveId: getItemDomId( selectedItem ),
} );
} ) as CompositeStore; // TODO, remove once composite APIs are public
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Necessary for the useStoreState call. Previously was any due to the locked nature of the private API.

@@ -3,6 +3,8 @@
*/
import clsx from 'clsx';
// TODO: use the @wordpress/components one once public
// eslint-disable-next-line no-restricted-imports
import { useStoreState } from '@ariakit/react';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WordPress/gutenberg-components WDYT - should we re-export this from @wordpress/components?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on what we decide around exposing stores in #63704. Better hold off for now

Copy link

Flaky tests detected in 13619bd.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10474021732
📝 Reported issues:

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smok tested with Storybook, and everything seems to be in order.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few suggestions to clean up Tabs code, but otherwise code changes LGTM

packages/components/src/tabs/tablist.tsx Outdated Show resolved Hide resolved
packages/components/src/tabs/tablist.tsx Outdated Show resolved Hide resolved
packages/components/src/tabs/tablist.tsx Show resolved Hide resolved
packages/components/src/tabs/tablist.tsx Outdated Show resolved Hide resolved
Co-authored-by: ciampo <[email protected]>
@tyxla
Copy link
Member Author

tyxla commented Aug 21, 2024

Thank you @Mamaduka and @ciampo 🙌

@tyxla tyxla enabled auto-merge (squash) August 21, 2024 13:11
@tyxla tyxla merged commit 9749a4a into trunk Aug 21, 2024
64 of 66 checks passed
@tyxla tyxla deleted the use/ariakit-use-store-state branch August 21, 2024 13:28
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 21, 2024
bph pushed a commit to bph/gutenberg that referenced this pull request Aug 31, 2024
…Press#64648)

* Components: Use useStoreState() instead of store.useState()

* CHANGELOG

* Cleanup TabList

Co-authored-by: ciampo <[email protected]>

---------

Co-authored-by: ciampo <[email protected]>

Co-authored-by: tyxla <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: ciampo <[email protected]>
swissspidy added a commit to swissspidy/media-experiments that referenced this pull request Oct 24, 2024
@@ -2,6 +2,7 @@
* External dependencies
*/
import * as Ariakit from '@ariakit/react';
import { useStoreState } from '@ariakit/react';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for using a separate import instead of Ariakit.useStoreState?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, no. I think it was caused by the automated way I addressed these changes. Let me fix these in a follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#67818 - thanks for spotting @Mamaduka 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants