Components: Add contextConnectWithoutRef() to bypass ref forwarding#43611
Merged
Components: Add contextConnectWithoutRef() to bypass ref forwarding#43611
Conversation
|
Size Change: -1 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
mirka
commented
Aug 25, 2022
Member
Author
There was a problem hiding this comment.
It would be cool if we could automatically detect the presence of a ref argument without having to manually set the noForwardRef option. However, I think it's impossible because we cannot reliably detect in JS whether a component function takes one argument (props) or two (props & ref). If we can't detect in JS, we can't do this conditional.
The next best thing would be to detect in TS whether the noForwardRef boolean is wrong, given the signature of the component function. I tried a bunch of ways with generics and conditional types, but unfortunately could not get it to work! It's a nice-to-have if anyone wants to try figuring it out.
Member
Author
There was a problem hiding this comment.
Yay, I found out a better way — TypeScript will now throw an error if the ref stuff is wrong. I updated the PR description.
This was referenced Aug 25, 2022
9686cea to
7ebd4f5
Compare
mirka
commented
Aug 26, 2022
mirka
commented
Aug 26, 2022
Comment on lines
+19
to
+22
| type AcceptsTwoArgs< | ||
| F extends ( ...args: any ) => any, | ||
| ErrorMessage = never | ||
| > = Parameters< F >[ 'length' ] extends 2 ? {} : ErrorMessage; |
Member
Author
There was a problem hiding this comment.
Turns out, TypeScript is (justifiably) loose about function arity. No wonder my previous attempts didn't work!
Here is a minimal repro in a TS Playground if you're interested in what's going on here and why it's necessary. (Solution courtesy of StackOverflow 😌)
ciampo
reviewed
Aug 29, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Great work here! This is not an easy task, and your changes definitely feel like an improvement
Previously, passing a ref-less function to contextConnect would cause dev mode React to throw a warning at runtime. This could be hard to notice if there are no render tests for the component.
Interesting! Were there any examples of such components?
Comment on lines
+73
to
+76
| ): WordPressComponentFromProps< Parameters< C >[ 0 ] > { | ||
| const WrappedComponent = options?.forwardsRef | ||
| ? forwardRef< any, Parameters< C >[ 0 ] >( Component ) | ||
| : Component; |
Contributor
There was a problem hiding this comment.
I wonder if we could simplify the code here by:
- using
ComponentProps< C >(or similar) instead of the more implicitParameters< C >[0] - assigning that type directly to
WrappedComponent— which would remove the need to specify an explicit function return type, and could potentially help with those@ts-expect-errorcomments later in the code
Member
Author
There was a problem hiding this comment.
Sounded hopeful but I couldn't make it happen 🙁 The blocker being, ComponentProps< C > needs C to be a JSXElementConstructor, while forwardRef() needs C to be a ForwardRefRenderFunction, and the two are not compatible 😵
Contributor
There was a problem hiding this comment.
🤯
I tried to use some infer magic and I came up with this
diff --git a/packages/components/src/ui/context/context-connect.ts b/packages/components/src/ui/context/context-connect.ts
index 0214ecc686..3a17897483 100644
--- a/packages/components/src/ui/context/context-connect.ts
+++ b/packages/components/src/ui/context/context-connect.ts
@@ -64,15 +64,21 @@ export function contextConnectWithoutRef< P >(
// This is an (experimental) evolution of the initial connect() HOC.
// The hope is that we can improve render performance by removing functional
// component wrappers.
+type InferredComponentProps< C > = C extends (
+ props: infer P,
+ ref: ForwardedRef< any >
+) => JSX.Element | null
+ ? P
+ : never;
function _contextConnect<
C extends ( props: any, ref: ForwardedRef< any > ) => JSX.Element | null
>(
Component: C,
namespace: string,
options?: ContextConnectOptions
-): WordPressComponentFromProps< Parameters< C >[ 0 ] > {
+): WordPressComponentFromProps< InferredComponentProps< C > > {
const WrappedComponent = options?.forwardsRef
- ? forwardRef< any, Parameters< C >[ 0 ] >( Component )
+ ? forwardRef< any, InferredComponentProps< C > >( Component )
: Component;
if ( typeof namespace === 'undefined' ) {But I'm not sure if it's a better solution than the current one — plus, should we wrap these props in ComponentPropsWithoutRef and/or ComponentPropsWithRef ?
It's quite entangled!
Member
Author
There was a problem hiding this comment.
Yeah, maybe we can add that utility type if we start to encounter it in more places.
7ebd4f5 to
4757bba
Compare
4757bba to
3539ef4
Compare
Member
Author
Fortunately I couldn't find any 😌 |
681b1b0 to
57e84cb
Compare
Member
Author
|
Rebased and ready for final review 🙏 |
ciampo
approved these changes
Aug 31, 2022
Comment on lines
+73
to
+76
| ): WordPressComponentFromProps< Parameters< C >[ 0 ] > { | ||
| const WrappedComponent = options?.forwardsRef | ||
| ? forwardRef< any, Parameters< C >[ 0 ] >( Component ) | ||
| : Component; |
Contributor
There was a problem hiding this comment.
🤯
I tried to use some infer magic and I came up with this
diff --git a/packages/components/src/ui/context/context-connect.ts b/packages/components/src/ui/context/context-connect.ts
index 0214ecc686..3a17897483 100644
--- a/packages/components/src/ui/context/context-connect.ts
+++ b/packages/components/src/ui/context/context-connect.ts
@@ -64,15 +64,21 @@ export function contextConnectWithoutRef< P >(
// This is an (experimental) evolution of the initial connect() HOC.
// The hope is that we can improve render performance by removing functional
// component wrappers.
+type InferredComponentProps< C > = C extends (
+ props: infer P,
+ ref: ForwardedRef< any >
+) => JSX.Element | null
+ ? P
+ : never;
function _contextConnect<
C extends ( props: any, ref: ForwardedRef< any > ) => JSX.Element | null
>(
Component: C,
namespace: string,
options?: ContextConnectOptions
-): WordPressComponentFromProps< Parameters< C >[ 0 ] > {
+): WordPressComponentFromProps< InferredComponentProps< C > > {
const WrappedComponent = options?.forwardsRef
- ? forwardRef< any, Parameters< C >[ 0 ] >( Component )
+ ? forwardRef< any, InferredComponentProps< C > >( Component )
: Component;
if ( typeof namespace === 'undefined' ) {But I'm not sure if it's a better solution than the current one — plus, should we wrap these props in ComponentPropsWithoutRef and/or ComponentPropsWithRef ?
It's quite entangled!
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
2 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?
Adds a separate
contextConnectWithoutRef()function so components can bypass theforwardRef()wrapping.Why?
Not all components need to forward refs.
How?
Moves the existing
contextConnectfunction to a private internal function, and instead exposes two separate functionscontextConnectandcontextConnectWithoutRef. TypeScript will now error when a ref-less function is passed tocontextConnect, and will surface a helpful warning message.Previously, passing a ref-less function to
contextConnectwould cause dev mode React to throw a warning at runtime. This could be hard to notice if there are no render tests for the component.Testing Instructions
✅ Types build without error