SlotFill: some code cleanups of the bubblesVirtually version #50133
Conversation
This file contains 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
jsnajdr
added
[Type] Code Quality
|
Size Change: -33 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Apr 28, 2023
There was a problem hiding this comment.
The SlotFills "package" also has two hooks named useSlot one is a public API and another used internally. It would be nice to rename latter and avoid confusion.
tyxla
approved these changes
Apr 28, 2023
There was a problem hiding this comment.
The cleanup looks good IMHO 👍 Thanks!
I agree with @Mamaduka that the naming conflict can be confusing, but I'd say that the entire situation with the two "standard" and "bubbles virtually" versions is a bit awkward too. Ideally, we'd have a single version that we could just configure.
Optionally, this could have a CHANGELOG entry, but up to you really.
There are just two usages of I'm also wondering if we can somehow get rid of |
jsnajdr
force-pushed
the
cleanup/bubbling-slot-fills
branch
from
6494585 to
7818c26
Compare
|
Flaky tests detected in 7818c26. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4837403363
|
Well,
That being said, I'd rather keep the hook and benefit from the pre-existing performance optimizations. We could of course do some measurements. |
jsnajdr
force-pushed
the
cleanup/bubbling-slot-fills
branch
from
7818c26 to
9a7587a
Compare
Yes, I'd like to keep all performance benefits, I'm just thinking aloud about what we really need About |
|
The "Verify changelog update" CI check is still unhappy about the changelog. Tt's because I added an entry for two PRs at once, referencing both of them: But the regexp in the check is more strict about parentheses, accepting only the |
|
Good point @jsnajdr, thanks for the ping. I see why it's confused and it'll be an easy enough fix. I'll get a PR together. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
After the successful #50098, here are some more code improvements in the
bubblesVirtuallyversion ofSlotFill:registryfor the context, we can save manyuseRefs anduseCallbacks by creating the registry as a constant object and then storing it, forever, in provider's state.useCallbacks inuseSlotby creating the bound API object as a singleapiobject, withuseMemo.Fillto move hook call into a separate statement, as is the common custom.