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

Composite: use internal context to consume composite store #64493

Merged

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 14, 2024

What?

Use React context to make sure that Composite sub-components use the correct composite context

Why?

Since multiple ariakit components use composite stores in their internal implementation, sometimes sub-components can consume the wrong composite store (see #63569 (comment))

How?

Add a new context that holds a reference to the correct composite store. Subcomponents get the store from context instead of relying on Ariakit internals

Testing Instructions

@ciampo ciampo self-assigned this Aug 14, 2024
@ciampo ciampo requested review from a team and diegohaz August 14, 2024 08:05
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Aug 14, 2024
@ciampo ciampo marked this pull request as ready for review August 14, 2024 08:06
@ciampo ciampo requested a review from ajitbohra as a code owner August 14, 2024 08:06
Copy link

github-actions bot commented Aug 14, 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: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>

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

@ciampo ciampo changed the title Composite: add internal context to make sure that sub-components consume the correct store Composite: use internal context to consume composite store Aug 14, 2024
Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 👍 🚀

I'd only add some documentation that includes that the store is passed down through context and should be consumed through useCompositeContext().

Also, let's make sure to update the instances that still provide store explicitly to Composite subcomponents, for example:

and

) {
const contextValue = useMemo(
() => ( {
store,
Copy link
Member

Choose a reason for hiding this comment

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

Let's document this so folks know that if they want to access the store inside a Composite, they can do it with the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added JSDocs and README section

@ciampo
Copy link
Contributor Author

ciampo commented Aug 14, 2024

I'd only add some documentation that includes that the store is passed down through context and should be consumed through useCompositeContext().

I've added Context as an exported sub-component of Composite, and added relative JSDocs and README section. I won't export useCompositeContext for now because consumers can simply use React.useContext( Composite.Context ). We can always add the custom hook if we think it's necessary.

Also, let's make sure to update the instances that still provide store explicitly to Composite subcomponents, for example:

Passing store explicitly is indeed redundant after this PR. The changes that you suggest will be applied in #63569

@ciampo ciampo enabled auto-merge (squash) August 14, 2024 13:13
Copy link

Flaky tests detected in b30b0c4.
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/10387982068
📝 Reported issues:

@ciampo ciampo merged commit b533bfd into trunk Aug 14, 2024
62 of 63 checks passed
@ciampo ciampo deleted the feat/stabilize-composite/pass-store-via-internal-context branch August 14, 2024 13:32
@github-actions github-actions bot added this to the Gutenberg 19.1 milestone Aug 14, 2024
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.

2 participants