Components: Add contextConnectWithoutRef() to bypass ref forwarding #43611
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
|
Size Change: -1 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
mirka
commented
Aug 25, 2022
| const { memo: memoProp = false } = options; | ||
| const { memo: memoProp = false, noForwardRef } = options; | ||
|
|
||
| let WrappedComponent = noForwardRef ? Component : forwardRef( Component ); |
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.
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
mirka
force-pushed
the
no-ref-context-connect
branch
2 times, most recently
from
9686cea to
7ebd4f5
Compare
mirka
changed the title
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; |
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
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; |
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
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 😵
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!
There was a problem hiding this comment.
Yeah, maybe we can add that utility type if we start to encounter it in more places.
mirka
force-pushed
the
no-ref-context-connect
branch
from
7ebd4f5 to
4757bba
Compare
mirka
requested review from
spacedmonkey,
ntwb,
nerrad,
ellatrix,
tellthemachines,
geriux,
ajlende,
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
mirka
force-pushed
the
no-ref-context-connect
branch
from
4757bba to
3539ef4
Compare
mirka
removed request for
juanmaguitar,
ntwb,
ajitbohra,
ryanwelcher,
nerrad,
ellatrix,
geriux,
ajlende,
tellthemachines and
fabiankaegy
Fortunately I couldn't find any 😌 |
mirka
force-pushed
the
no-ref-context-connect
branch
from
681b1b0 to
57e84cb
Compare
|
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; |
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!
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.
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