Composite: remove store prop and useCompositeStore hook#64723
Merged
Conversation
ciampo
force-pushed
the
feat/stabilize-composite/remove-store-prop
branch
from
2946983 to
d68c3e1
Compare
|
Size Change: -301 B (-0.02%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
ciampo
commented
Aug 22, 2024
ciampo
commented
Aug 22, 2024
ciampo
commented
Aug 22, 2024
ciampo
commented
Aug 22, 2024
ciampo
commented
Aug 22, 2024
ciampo
commented
Aug 22, 2024
ciampo
commented
Aug 22, 2024
Contributor
Author
|
@WordPress/gutenberg-components can you take a look at the proposed approach? If we agree on moving forward, I'll tackle the missing changes and make it ready to review properly. |
tyxla
reviewed
Aug 23, 2024
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
The approach seems very sensible for the most part 👍
It appears we might need to expose some additional utilities to work with parts of the store, instead of the entire store as is currently necessary in dataviews.
If we continue to work directly with the store in dataviews, it feels like we're defeating the purpose of concealing the store prop in the first place. Would be nice if we can explore some alternatives, and expose only parts of the store that we'll actually need.
Would love to hear feedback from @mirka as well.
Contributor
Author
|
Ideally, I'd like to find ways for consumers of the component to achieve the desired behaviour without the need for the For example:
I'll see if I can apply these ideas into practice in this PR |
ciampo
force-pushed
the
feat/stabilize-composite/remove-store-prop
branch
from
ba27ce2 to
25dc6da
Compare
Contributor
Author
|
I pushed a few commits where I refactored some of the the most involved usages of the composite store — they now rely on controlling the There are still some usages left (and one usage to polish), but I'd say that this is looking promising and I think that we may be able to move away from using |
ciampo
force-pushed
the
feat/stabilize-composite/remove-store-prop
branch
from
c63db73 to
1a7d717
Compare
ciampo
commented
Aug 26, 2024
Contributor
Author
This was referenced Aug 27, 2024
Contributor
Author
|
@WordPress/gutenberg-components I'd appreciate some feedback on this approach, as it's currently blocking any further progress on stabilizing |
mirka
reviewed
Aug 30, 2024
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
I'm assuming we're doing more in-depth reviews in the partial PRs later, but at a high-level this certainly looks viable. I'm still worried about when we'll need any of the store methods, but it seems ok for now?
Contributor
Author
|
Thank you, @mirka! There seems to be consensus around moving forward with this approach. [EDIT: the list was moved to the PR description for better discovery] Once all of the above are merged, I can rebase this PR including only changes to |
Member
|
Thanks for working on it @ciampo 👍 the approach makes sense to me as well. Will take a look at the more focused PRs. |
This was referenced Sep 2, 2024
ciampo
force-pushed
the
feat/stabilize-composite/remove-store-prop
branch
from
a2fe333 to
1d9e82c
Compare
ciampo
force-pushed
the
feat/stabilize-composite/remove-store-prop
branch
from
ef9f741 to
09cb4ad
Compare
ciampo
commented
Sep 4, 2024
| rtl, | ||
| } ); | ||
|
|
||
| const store = storeProp || newStore; |
Contributor
Author
There was a problem hiding this comment.
This is the main (only) runtime change — we don't accept a store prop anymore on Composite. Although it shouldn't really make a difference, since we've already migrated all usages that were passing a store prop.
ciampo
commented
Sep 4, 2024
Contributor
Author
There was a problem hiding this comment.
Storybook changes:
- Moved from
StoryFntoStoryObjstyle for Storybook examples; - Changed the main component from
UseCompositeStorePlaceholdertoComposite🎉 - Removed the Storybook utils, which are not needed anymore
- Added
Composite.Contextas a subcomponent
ciampo
commented
Sep 4, 2024
| </SlotFillProvider> | ||
| ); | ||
| } | ||
| Provider.displayName = 'SlotFillProvider'; |
Contributor
Author
There was a problem hiding this comment.
For better Storybook code snippets and debugging with React DevTools
ciampo
commented
Sep 4, 2024
| // `onFocusVisible` needs the `Composite` component to be focusable, | ||
| // which is implicitly achieved via the `virtualFocus: true` option | ||
| // in the `useCompositeStore` hook. | ||
| // which is implicitly achieved via the `virtualFocus` prop. |
Contributor
Author
There was a problem hiding this comment.
This comment is out of date after the recent refactor
ciampo
commented
Sep 4, 2024
Contributor
Author
There was a problem hiding this comment.
Using .tsx instead of .ts is more Storybook-friendly for types extraction.
ciampo
added
[Type] Code Quality
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Contributor
Author
mirka
approved these changes
Sep 4, 2024
Member
mirka
left a comment
mirka
left a comment
There was a problem hiding this comment.
Beautiful, great work on this! The rtl thing can be kicked to a follow-up, by the way.
| /** | ||
| * The component store, used for advanced usage of the component. | ||
| */ | ||
| store?: unknown; |
Member
There was a problem hiding this comment.
Sorry if this was already discussed somewhere — why is this unknown now? Might need a code comment since it was non-obvious to me.
Contributor
Author
There was a problem hiding this comment.
I think it was discussed in one of the (many) inline comments, definitely quite easy to miss 😅
We thought that, in order to discourage store usage, we'd change the type to unknown — which still allows TypeScript to correctly validate scenarios like context forwarding (check the "With Slot Fill" example), but it wouldn't give insight into what store is and does.
I added a comment for now to clarify.
What do you think? Too much?
Happy to make changes in a follow-up.
Member
|
Great to see this one land! 🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Remove the
useCompositeStoreexport and thestoreprop from theCompositecomponentWhy?
See #63704 (comment)
How?
useCompositeStorefrom the component's exported APIs;storeprop from theCompositecomponent;storeprop fromComposite.Context;All usages of the
useCompositeStorehook and thestoreprop were removed in separate PRs:Testing Instructions
No runtime changes, apart from this one.
useCompositeStoreleft, apart from theCompositecomponent itself;storeprop toCompositeor its sub-components;Composite's Storybook docs are up to date and the examples work as expected;useCompositeStoreanymore)