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

Tooltip: Fix Ariakit tooltip store usage #61858

Merged
merged 2 commits into from
May 22, 2024

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented May 22, 2024

What?

This PR fixes how we use the Ariakit.useTooltipStore hook in the Tooltip component.

Why?

Previously, we were circumventing an ESLint error because the hook was conditionally called. I recall I suggested this workaround, so I'm glad I'm the one to suggest removing it now (see #57202 (comment)).

The workaround is no longer necessary since the hook is no longer called conditionally.

The motivation is that I'm fixing it to resolve a couple of ESLint errors that were raised by the React Compiler ESLint plugin in #61788

How?

Just using the hook directly.

Testing Instructions

Verify the static analysis check is still green.

Testing Instructions for Keyboard

None

Screenshots or screencast

None

@tyxla tyxla added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels May 22, 2024
@tyxla tyxla requested review from Mamaduka, jsnajdr and a team May 22, 2024 10:25
@tyxla tyxla self-assigned this May 22, 2024
@tyxla tyxla requested a review from ajitbohra as a code owner May 22, 2024 10:25
Copy link

github-actions bot commented May 22, 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 <tyxla@git.wordpress.org>
Co-authored-by: diegohaz <hazdiego@git.wordpress.org>

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

Copy link

Flaky tests detected in 98fa4cc.
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/9189637481
📝 Reported issues:

@tyxla tyxla merged commit 6e8c261 into trunk May 22, 2024
63 checks passed
@tyxla tyxla deleted the cleanup/tooltip-ariakit-store-usage branch May 22, 2024 11:32
@github-actions github-actions bot added this to the Gutenberg 18.5 milestone May 22, 2024
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.

Thanks, @tyxla!

We also need to figure out how to handle warnings/errors for Ariakit's store hooks.

const compositeStore = useCompositeStore();
const activeId = compositeStore.useState( 'activeId' );
@tyxla
Copy link
Member Author

tyxla commented May 22, 2024

We also need to figure out how to handle warnings/errors for Ariakit's store hooks.

const compositeStore = useCompositeStore();
const activeId = compositeStore.useState( 'activeId' );

Definitely. Might need some additional wrapper, in Ariakit or here, in the package. One simple solution I can think of is having a separate useCompositeStoreState( store ) hook that accepts a store argument and will return the internal state of that store based on that store argument. Might also need some name shuffling to prevent ESLint from being sad again.

@tyxla
Copy link
Member Author

tyxla commented May 22, 2024

@diegohaz @DaniGuardiola is this a problem you've encountered and/or thought about already in Ariakit?

@diegohaz
Copy link
Member

I'm not sure I'm aware of that problem. Could you elaborate?

@tyxla
Copy link
Member Author

tyxla commented May 22, 2024

We're trying out the new React Compiler ESLint plugin in #61788

It will complain that hooks can't be just dynamically declared or assigned somewhere. So in code like @Mamaduka provided above:

const compositeStore = useCompositeStore();
const activeId = compositeStore.useState( 'activeId' );

it will complain because the useState hook reference can potentially change, and that's against the rules of hooks - more details here.

To solve this issue, useState must be recognized by the React Compiler as a stable, static reference. My conclusion is that we might need a different API for that (as I roughly suggested above), but I'm curious if you have any other ideas.

@Mamaduka
Copy link
Member

Mamaduka commented May 22, 2024

@diegohaz, the following code triggers a warning/error when enabling the new React compiler ESLint rules.

[2] /home/runner/work/gutenberg/gutenberg/packages/components/src/alignment-matrix-control/index.tsx
[2]   71:19  error  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

const compositeStore = useCompositeStore( {
defaultActiveId: getItemId( baseId, defaultValue ),
activeId: getItemId( baseId, value ),
setActiveId: ( nextActiveId ) => {
const nextValue = getItemValue( baseId, nextActiveId );
if ( nextValue ) {
onChange?.( nextValue );
}
},
rtl: isRTL(),
} );
const activeId = compositeStore.useState( 'activeId' );

@diegohaz
Copy link
Member

I see. Ariakit has an experimental useStoreState function (currently exported by @ariakit/react-core/utils/store) that can be used like this:

const activeId = useStoreState(compositeStore, 'activeId');

This is essentially what compositeStore.useState does internally. There are no plans to promote this API to @ariakit/react soon, but it might happen if this is a general issue.

@tyxla
Copy link
Member Author

tyxla commented May 22, 2024

There are no plans to promote this API to @ariakit/react soon, but it might happen if this is a general issue.

I think that might be the case. Thanks for considering it. Also, cc @DaniGuardiola who's working on the Ariakit upgrade in #60992

@DaniGuardiola
Copy link
Contributor

DaniGuardiola commented May 22, 2024

@tyxla I'm not sure this PR is doing anything other than trick eslint into thinking there's no hook to analyze. The React linting tule is just too dumb to identify namespaces hooks as such, but it is still a hook and it should still respect the rules of hooks that eslint complained about.

Separately, we should wrap these hooks into ones that throw if the context is not present, so that no conditional logic is necessary (this is also what we discussed regarding Tabs).

Edit: I think I misunderstood, will check again once I'm back from lunch.

carstingaxion pushed a commit to carstingaxion/gutenberg that referenced this pull request Jun 4, 2024
* Tooltip: Fix Ariakit tooltip store usage

* CHANGELOG

Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: diegohaz <hazdiego@git.wordpress.org>
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
* Tooltip: Fix Ariakit tooltip store usage

* CHANGELOG

Co-authored-by: tyxla <tyxla@git.wordpress.org>
Co-authored-by: diegohaz <hazdiego@git.wordpress.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
4 participants