[compose]: Fix type for createHigherOrderComponent
#40984
Conversation
|
After a lot of trial and error I was unable to get to where I wanted to be but I think there's an issue with the fact that the type parameters can't be part of If you move some of the type signatures to the inside of Would love some help figuring this one out. Turns out that as @jsnajdr pointed out, there are almost no actual restrictions on the final result from a higher-order component and the restrictions come from the implementation inside of it. For example, one higher-order component may want to be the sole-supplier of a prop and therefore forbid passing it down to the wrapped component (see The type changes here more-closely reflect that I think, and are closer to what I think is right than we had in #37795 or its parent. cc: @sirbrillig |
gziolo
added
[Type] Code Quality
|
These types still seem less correct or comprehensive than the ones I originally wrote. For example, with the original types, you get this error with the ifCondition HOC:
With the previous changes and these ones you don't get any errors like that. |
There was a problem hiding this comment.
@dmsnell I think the main blocker of this work is that we don't have clear shared requirements on how the createHigherOrderComponent should behave.
When I read your comments like # or # I vaguely understand what issues you're describing, but I'm not really sure. When trying to type HOCs myself, I also run into some issues, but are we really talking about the same thing? Don't know.
What if we created a types unit test file, like test/types.tsx, and put usage examples there? Like "I want to write a HOC like this and expect the types to be X and Y" or "when I write this I expect that the compiler reports an error."
We're done when the unit test file passes typechecking without errors, and with @ts-expect-error we can even check there are errors where we want them to be.
Our expectations would be written down as code, and I believe that would clarify the collaboration a lot.
| infer Props | ||
| > | ||
| ? Props | ||
| : never; |
There was a problem hiding this comment.
There's a React.ComponentProps<C> type that extracts the props type from a component type, could we use that?
There was a problem hiding this comment.
thank you. I swapped to ComponentProps in cfa07f3
had looked for it but didn't find that when I initially wrote the patch.
| export function createHigherOrderComponent( | ||
| mapComponent: ( Inner: ComponentType< any > ) => ComponentType< any >, | ||
| modifierName: string | ||
| ): typeof mapComponent { |
There was a problem hiding this comment.
Now when createHigherOrderComponent and mapComponent are no longer generic functions, with type parameters, that means that createHigherOrderComponent erases the type of mapComponent and returns a generic any => any mapper.
For example, if I pass ComponentType<MyProps> to pure, I should get a ComponentType<MyProps>, component of the same type. But I get ComponentType<any>.
|
In #37795 (comment) @dmsnell writes:
and I'm now starting to understand the motivation for #37795 because I found that the type PropInjectingHigherOrderComponent<TRemovedProps> =
<TProps extends TRemovedProps>( Inner: ComponentType< TProps > ) =>
ComponentType< Omit< TProps, keyof TRemovedProps > >;The idea is that, for example, a type InstanceIdProps = { instanceId: string | number };and that means the type of But there's subtle bug. Consider this example: type InstanceIdProps = { instanceId: string | number };
const withInstanceId: PropInjectingHigherOrderComponent<InstanceIdProps> = ...;
type FooProps = { foo: string, instanceId: string };
function Foo({ foo, instanceId }: FooProps) {
return <div id={instanceId.slice(0,4)}>{foo}</div>;
}
const WrappedFoo = withInstanceId(Foo);TypeScript will happily accept this code, but it has a serious type error and can crash at runtime! Can you spot it? The Wasn't the But const fooProps: FooProps = { foo: 'hello', instanceId: 'world' };
const idProps: InstanceIdProps = fooProps;This assignment is perfectly fine because But our HOC does assignments in the opposite direction, taking value of |
I found that First, there is a Second, instead of Combining the two ideas together, we get: Now Hope this is helpful and moves us forward. For today, the TypeScript center in my brain is totally burned out and I'm moving on to something else 🙂 |
When refactoring `createHigherOrderComponent` in #37795 an incorrect type was introduced in an attempt to correct the previous type. The fix in that patch was focused on the narrow use-case of a _prop-injecting higher-order component_. In this patch we're lifting the type signature into the function which calls `createHigherOrderComponent`. There's a limitiation I'm not sure how to get past when adding the type parameters to the generalized interface; that is, the function _produced_ by `createHigherOrderComponent` needs its own type parameters for input and output specified by the given `mapComponent` mapper. This change require more type juggling when defining a new higher-order component but removes the arbitrary constraint on what props may be passed into or come out of the higher-order component.
dmsnell
force-pushed
the
types/fix-create-higher-order-component
branch
from
32536ad to
8040756
Compare
@jsnajdr yes! I think that's exactly right, and truth be told the more I look into it the less power I see export const createHigherOrderComponent = ( mapper, name ) => ( Inner ) => {
const Outer = mapper( Inner );
Outer.displayName = hocName( name, Inner );
return Outer;
};
const hocName = ( name: string, Inner: ComponentType< any > ): string => {
const inner = Inner.displayName || Inner.name || 'Component';
const outer = upperFirst( camelCase( name ) );
return `${ outer }(${ inner })`;
};When I was first trying to understand this function I was very confused. Do you think we could roll back the types on this and leave it untyped for now? Every time I try and capture and preserve the type of In the meantime I could see us extracting |
The
I think it's useful to divide the problem into two separate ones:
|
|
@jsnajdr I haven't yet dug into the exclusionary types you suggested, but thank you for posting those as I think we'll need them. In 0208be8 I have tried again to infer the type of Each actual higher-order component is free to define its own constraints and @sarayourfriend I can't see enough in your screenshot to know what's going on or what you expect there, but you might check again. In this iteration we show∂ be able to fully-define in each HOC what they want, so we can dig in. The plan is to get to the |
|
Size Change: +1.58 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
Sorry, the screenshot was bad. I've tried the same code on the latest version and it still does not catch the type error. This is the code I'm using to exercise const Comp = ({ foo }: { foo: string }) => <div>{foo}</div>
const ifBar = ifCondition((p: { bar: string }) => !!p.bar)
const IffComp = ifBar(Comp)calling The original version errored if you passed a component to Neither of these is actually correct, now that I think about it, but it's probably a bad |
| /> | ||
| ), | ||
| 'instanceId' | ||
| ); |
There was a problem hiding this comment.
There are several things wrong about the withInstanceId types:
extends ComponentProps< any >is redudant becauseComponentPropsnever gets a reasonable component type to infer props from. The result is either{}orany.- the
withInstanceIdHOC accepts also components that don't acceptinstanceIdas a prop, which is an undetected type error. - the
withInstanceIdHOC returns a component that accepts theinstanceIdprop, which is an error, too. The outer component doesn't accept that prop, because it's supposed to be injected.
|
Today I tried to do this: type InstanceIdProps = { instanceId: string | number };
type Combine< P, I > = Omit< P, keyof I > & I;
function addInstanceId<
C extends ComponentType< Combine< ComponentProps< C >, InstanceIdProps > >
>( WrappedComponent: C ) {
return ( props: Omit< ComponentProps< C >, keyof InstanceIdProps > ) => {
const instanceId = useInstanceId( WrappedComponent );
const wrappedProps = { ...props, instanceId } as ComponentProps< C >;
return <WrappedComponent { ...wrappedProps } />;
};
}
const withInstanceId = createHigherOrderComponent( addInstanceId, 'instanceId' );The types of But there are two errors reported. First, TypeScript doesn't accept Second, |
|
I found some tricks in |
while reviewing I did consider this and thought it shouldn't be an error, particularly because the this reminds me that there are two levels interacting here that probably have been keeping this confusing: every HOC has a different type from every other HOC and that's separate from the type of can we think of a way to split this so that we can free the individual HOCs' types from @jsnajdr will still take me some time to process what you wrote but thank you for continuing to work on this with us. |
The HOC, however, passes all props down to the wrapped component, there is no way to opt out of that. |
True, but not from a type perspective. If you read on in my comment I speculate that the issue isn't actually that the I'd say in this sense that the original implementation was incorrect, it should not be an error, but it also shouldn't produce a uselessly wide type. |
What I think the accurate type is follows something like this:
This sounds like what you found with the Redux typings.
This is where I'm kind of giving up until we have everything truly worked out. I don't know how to avoid the false error without widening, but one thing we can do is allow the actual HOCs (the |
I believe that any HOC written with the When there is a function that accepts const Foo = ( props: { foo: string } ) => null;I can call it with a const theProps = { foo: 'x', instanceId: 1 };
Foo( theProps ); // this is fineBecause the type of The same is acceptable when written as JSX: return <Foo { ...theProps } />Nobody demands that the However, there's one situation where TypeScript actually demands this and which confused me a lot until I realized what's going on: Foo( { foo: 'x', instanceId: 1 } );Here TypeScript will report an error: Although the parameter has a valid type (subtype of the param type), TypeScript won't allow it because it's written as object literal. Once I introduce a helper The same error will happen with JSX written as: return <Foo foo="x" instanceId={ 1 } />Here the error will be different: But it's exactly the same situation as the object literal, it's just the error messages that are different for the JSX and non-JSX code path. We need to keep this in mind when writing HOCs, because const wrappedProps = { ...props, injectedProp };
return <Wrapped { ...wrappedProps } />;and return <Wrapped { ...props } injectedProp={ injectedProp } />both compile to exactly the same code, but one is TS-ok and the other is TS-error. |
The |
Oh boy. Do you understand why it does this? I guess on one hand it makes sense to say "hey, you're typing these things by hand here, but you know that property will be ignored right? maybe it's a typo?" but on the other hand it seems odd when TS is happy if you split that inline expression into a variable holding a literal value and a call referencing the value. In reflecting on what you wrote I do believe I've seen this a number of times without realizing that the important distinction is whether or not it's a literal value in an inferred context like that. |
It sounds like a reasonable thing to do: you can always change the literal to remove the excess properties, and as the literal is used only once at that particular place, it doesn't affect anything else. So TypeScript forces you to remove them. If you split a variable, TypeScript would need to check that the variable is really used only once, and that's additional flow analysis and a much more complex check to do. What I don't like about this and what I believe is fixable, is the error message in the JSX case. Assigning object literals, TS tells you exactly what it doesn't like: But in the JSX case the same error is more obfuscated: |
|
closing in favor of #41138 |
What?
When refactoring
createHigherOrderComponentin #37795an incorrect type was introduced in an attempt to correct
the previous type. The fix in that patch was focused on
the narrow use-case of a prop-injecting higher-order component.
In this patch we're lifting the type signature into the function
which calls
createHigherOrderComponent. There's a limitiationI'm not sure how to get past when adding the type parameters to
the generalized interface; that is, the function produced by
createHigherOrderComponentneeds its own type parameters forinput and output specified by the given
mapComponentmapper.This change require more type juggling when defining a new
higher-order component but removes the arbitrary constraint
on what props may be passed into or come out of the higher-order
component.
Why?
The type in
createHigherOrderComponentwas overly-restrictive.How?
Moves the type parameters from
createHigherOrderComponenttothe code where it's called.
Testing Instructions
As a type-only change there should be no impact on the built artifacts.
No tests should start failing.
Review the approach taken for the type of
createHigherOrderComponentas well as how this demands a change in the way calling code is
typed.
Note that if no type is provided at the call-site then the output
of
createHigherOrderComponentwill beComponentType<any>, whichstill should be valid even if it doesn't track the appropriate
Props.The proper way to handle this is to look at the examples and type the
variable declaration itself.