Components: remove createPrivateSlotFill function #67238
Merged
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
jsnajdr
requested review from
Mamaduka,
youknowriad,
mirka,
ciampo and
tyxla
|
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. |
|
Size Change: -46 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
mirka
reviewed
Nov 23, 2024
There was a problem hiding this comment.
It looks like this idea was already floated in the original issue. The arguments given against it were code deduplication, consistency, and convenience. I don't have enough awareness of how private slot fills are used across the app, but judging only from the code changes in this PR, it doesn't seem much more complicated or boilerplate-y. It's arguably more simple because it exposes the standard privacy mechanism (Symbol) rather than blackbox it.
Would the consistency and convenience angle be mitigated by documentation perhaps? What if we documented how to do private slot fills in the README and reference guide?
| @@ -84,17 +84,12 @@ export function createSlotFill( key: SlotKey ) { | |||
| props: DistributiveOmit< SlotComponentProps, 'name' > | |||
| ) => <Slot name={ key } { ...props } />; | |||
| SlotComponent.displayName = `${ baseName }Slot`; | |||
| // deprecated legacy property, should use `slotFill.name` instead of `slotFill.Slot.__unstableName` | |||
There was a problem hiding this comment.
This will make it show up in IntelliSense when accessed.
Suggested change
| // deprecated legacy property, should use `slotFill.name` instead of `slotFill.Slot.__unstableName` | |
| /** @deprecated Use `slotFill.name` instead of `slotFill.Slot.__unstableName`. */ |
There was a problem hiding this comment.
Thanks for the ping 👍
I don't have any strong opinions on the API for creating private SlotFills. As @mirka notes there were discussions around it on the original PR as well as some offline conversations.
The end result, createPrivateSlotFill, was the consensus at the time. That also involved some consideration that we weren't certain on the API or whether we even wanted to set public examples of defining private slot fills.
wpdirectory.net says that there are usages in plugins (Kubio), but they are in a safe form: ( createPrivateSlotFill || createSlotFill )( name ).
To my last point above, createPrivateSlotFill was a private API itself, so there should be no uses of it outside the project. At least none that didn't come with the promise that it might be removed or break.
Would the consistency and convenience angle be mitigated by documentation perhaps? What if we documented how to do private slot fills in the README and reference guide?
Sounds like a plan to land this. If we're pushing this more into the public space, should it also then be documented via the Storybook too?
Since the original PR was landed, the slot/fill implementation got rewritten into TypeScript (#51350) where the Additionally I also pushed a commit now where I update the
Today this API is used at exactly two places, one in
Nice idea, implemented 👍 |
|
One motivation for this PR was when I was reviewing #66825 and noticed that the dataviews + components bundle includes the entire Removing or stabilizing private APIs can also have impact on bundle size. After this PR |
Mamaduka
approved these changes
Nov 25, 2024
There was a problem hiding this comment.
Thank you, @jsnajdr!
The reason for the changes here makes sense. It's also nice to see the name property stabilized.
Props for updating the documentation 🦸♂️
P.S. In the future, we could also include examples of when to use bubblesVirtually=true vs. bubblesVirtually=false. I always forget it 😅
We should almost always use |
However, plenty of |
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.
In #49819 @aaronrobertshaw added private API function
createPrivateSlotFillbut on a closer look, I don't think this function does anything that the publiccreateSlotFillcouldn't do. You can create a slotfill with aSymbolname:and if you don't expose any of the return value fields publicly, nobody unauthorized can use that slot. All you need for privacy is that the name is a symbol, because different instances of a symbol are not equal even if they have the same
description:If the name is a string then anyone who knows the name can use it:
In this PR I'm removing
createPrivateSlotFilland replacing usages withcreateSlotFill( Symbol() ). The return value ofcreateSlotFillhas a newnamefield that contains the name. This is commonly used withuseSlotFills( name ). This field is supposed to replace the semi-privateslotFill.Slot.__unstableNamethat's been used for this purpose until now.wpdirectory.net says that there are usages in plugins (Kubio), but they are in a safe form:
( createPrivateSlotFill || createSlotFill )( name ).