Improves the types of createHigherOrderComponent and its usages #41138
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
requested review from
dmsnell and
sarayourfriend
and removed request for
ajitbohra
|
Size Change: +10 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
What I discovered in #40984 is that I think we might want to avoid adding a For the prop-injecting HOCs with default overrides we shouldn't demand that its own props can't be passed in; for the prop-injecting HOCs whose props are purely for the HOC we shouldn't demand that the inner component must receive them. This all makes me think we should either leave out any/all types for HOCs or try and find a way to deprecate const WithName = <C extends ComponentType>(component: C, name: string, …): C => {
…
return component;
}To me this approach lets people focus on the types of the individual HOCs which I think is the only reasonable place given that they all might have different (type-able) restrictions, and this helper function we have doesn't have to act like it's doing more than it is, which is mutate the thing passed into it by adding a single property. |
As I wrote in #40984 (comment), these are not issues with the types themselves, but with how TypeScript doesn't allow extra props in object literals or in "JSX literals." When there is a wrapped component: const WrappedFoo = withInstanceId( Foo );Then the return <WrappedFoo instanceId={ null } />is still legal from a type compatibility point of view. The reason why TypeScript rejects this is that it has a special rule that "literal" values can't have extra fields. Writing the same as: return <WrappedFoo { ...{ instanceId: null } } />is OK because the "literal" syntax has been removed. It's also true that if I wanted to write a HOC that injects a value only when not supplied, typically like: const withSite = ( Inner ) => ( { siteId, ...props } ) => {
const injectedSiteId = useSelector( getSelectedSite );
return <Inner { ...props } siteId={ siteId ?? injectedSiteId } />;
}then the |
dmsnell
approved these changes
Jun 2, 2022
|
|
||
| return <OriginalComponent { ...props } />; | ||
| return ( | ||
| // @ts-ignore |
There was a problem hiding this comment.
can we see if we can get rid of this or replace it with ts-expect-error (or whatever the expect-error syntax is?)
There was a problem hiding this comment.
I think that ts-ignore is more precise here because there is not really any type error: the OriginalComponent is always getting correct props whatever its type (the C type variable) is. It's just TypeScript is not able to see that. Because it involves some reasoning about C that it cannot do. In future, it might be able to and then stops reporting an error.
annezazu
added
[Type] Code Quality
5 tasks
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.
@dmsnell @sarayourfriend This is my attempt at correcting the types of
createHigherOrderComponentand its applications, especially HOCs that inject props.The
createHigherOrderComponenttype has two type parameters, the type of the input and output components. They both need to be a subtype ofComponentType<any>, so that we can read and write their.displayNamefields. There are no further constraints an in practice, the types are always easily inferred from the type ofmapComponent.This is a blueprint for creating a prop-injecting HOC, with some new helper types:
The
C extends WithInjectedProps< C, FooProps >constraint checks thatC, type of the wrapped component, supports all the props that are going to be injected. IfBardoesn't support afooprop, thenwithFoo( Bar )will be a type error.The result of
withFoo( C )has typeWithoutInjectedProps< C, FooProps >, i.e., all the props ofCwith injected props removed.The
@ts-ignoreI had to do on the<Inner>JSX, that's a mysterious behavior that's probably a bug either in TypeScript itself or in@types/react. This is a minimalistic example for it that you can paste into TS playground:There will be a type error reported:
even though it's perfectly fine to pass a
fooprop toComp. Maybe I'll be filing a bug.Let me know what you think.