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

SlotFills: Use state for registry initialization #61802

Merged
merged 1 commit into from
May 21, 2024

Conversation

Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented May 20, 2024

What?

PR updates SlotFill providers to initialize context values via useState lazily. The code used the same initialization method but changed during TS refactoring - 821f4d4.

Why?

Testing Instructions

CI checks should be green.

Testing Instructions for Keyboard

Same.

@Mamaduka Mamaduka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels May 20, 2024
@Mamaduka Mamaduka self-assigned this May 20, 2024
@Mamaduka Mamaduka requested a review from ajitbohra as a code owner May 20, 2024 16:36
@Mamaduka Mamaduka requested review from jsnajdr, torounit and tyxla and removed request for ajitbohra May 20, 2024 16:36
@@ -98,7 +98,7 @@ function createSlotRegistry(): SlotFillBubblesVirtuallyContext {
export default function SlotFillProvider( {
children,
}: SlotFillProviderProps ) {
const registry = useMemo( createSlotRegistry, [] );
const [ registry ] = useState( createSlotRegistry );
Copy link
Member

Choose a reason for hiding this comment

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

Why can't it be useMemo( () => createSlotRegistry(), [] )?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, read those articles, that's interesting.

Copy link

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: Mamaduka <[email protected]>
Co-authored-by: ellatrix <[email protected]>

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

@ellatrix
Copy link
Member

There's probably quite a few places where we're misusing useMemo

@Mamaduka
Copy link
Member Author

There's probably quite a few places where we're misusing useMemo.

Unfortunately, it can't be avoided in large projects.

I've seen some unnecessary memoizations in the code. Hopefully, we'll have to worry less about this when the project starts using the react-compiler.

@Mamaduka Mamaduka merged commit c706195 into trunk May 21, 2024
71 of 72 checks passed
@Mamaduka Mamaduka deleted the update/slot-fill-context-initializers branch May 21, 2024 05:09
@github-actions github-actions bot added this to the Gutenberg 18.5 milestone May 21, 2024
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 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] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants