Compose: Rework types of createHigherOrderComponent for closer match to API #37795
Conversation
|
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
| // eslint-disable-next-line no-restricted-imports | ||
| import type { ComponentType } from 'react'; |
There was a problem hiding this comment.
Instead of telling ESLint to shut up, how about we export that type from our package instead?
There was a problem hiding this comment.
Not really telling it to "shut up" 😆
There are almost ninety other instances of disabling this error for a type import. I'm happy to try and get back to adjust @wordpress/element but it's still a JavaScript file without exported types. I'd prefer to not hold up this PR while waiting to change @wordpress/element and update all of those other existing cases.
Thoughts?
There was a problem hiding this comment.
It's disabling this rule:
Lines 84 to 88 in cc569b2
It was discussed several times before. Ideally, we would re-import APIs we use from @wordpress/element for consistency with how React is used. However, it never seemed to be a simple task with TypeScript. People started disabling this rule when importing rules. We should instead seek ways to update this rule so it doesn't error for types.
There was a problem hiding this comment.
It looks like it's a simple as using a different rule for TS files:
| @@ -72,8 +71,9 @@ const withSafeTimeout: PropInjectingHigherOrderComponent< TimeoutProps > = creat | |||
| ...this.props, | |||
| setTimeout: this.setTimeout, | |||
| clearTimeout: this.clearTimeout, | |||
| }; | |||
| return <OriginalComponent { ...( props as TProps ) } />; | |||
| } as TProps; | |||
There was a problem hiding this comment.
Why do we need this type assertion?
There was a problem hiding this comment.
I'm not totally confident on my understand here, but I believe that it's because of the Omit. What I'm trying to convey is that the wrapped component doesn't want the timer functions to be passed in from the outside, but it still provides them to the wrapped component. I think there could be an issue here with React saying that props could contain any number of properties and TypeScript wants to complain that we might get ones we don't want or expect.
There was a problem hiding this comment.
Why do we need this type assertion?
It's described in this TS issue: microsoft/TypeScript#35858 (comment)
Quote:
TS isn't capable of the higher-order reasoning needed to understand that
Exclude<T, k> & { [k]: T[k] }is equivalent to T.
If you observe how the type of props is Omit<TProps, ...> and how the value of local props value is constructed with { ...this.props, ...}, it's exactly the same situation.
ZebulanStanphill
added
[Package] Compose
|
This is awesome. I did a little testing on this branch and everything I tried seemed to work great. @jsnajdr and I spent a considerable amount of time trying to find a solution for this, I'm glad there was something simpler and much more straightforward to use possible! Really nice work. Will be happy to review again once it's converted from a draft to a full PR. |
dmsnell
force-pushed
the
types/createHigherOrderComponent
branch
from
2bc2411 to
f31386c
Compare
There was a problem hiding this comment.
Looks great to me! I would love a second opinion from @jsnajdr who tends to see things I miss! But otherwise thanks so much for finding this significant improvement to the usability of this utility.
…to API The existing types in `createHigherOrderComponent<TOuter, TInner>` are somewhat _surprising_ because they require defining what possible prop types are allowed for components wrapped by the higher order component it produces. Additionally it doesn't concern itself with preventing the wrapped component from accepting or demanding props it itself provides. In this patch we're reworking the types to only require a single type parameter, which is the type of props that the higher order component injects into the wrapped component. The prop types for the wrapped component are inferred only when wrapping a component, and the injected props are removed from the external interface to avoid confusion.
dmsnell
force-pushed
the
types/createHigherOrderComponent
branch
from
1af7598 to
7971f9f
Compare
|
Rebased against the fix to |
|
cc: @sirbrillig keep an eye out for this in the next release ✅ |
|
Merging for lack of further review. Should anyone notice any problems with this please don't hesitate to ping me or ask for further refinement. |
|
I'm just now getting around to updating this package in a 3rd party repo. We have this use case: export const withLocale = createHigherOrderComponent< { locale: string } >( ( InnerComponent ) => {
return ( props ) => {
const locale = useLocale();
return <InnerComponent { ...props } locale={ locale } />;
};
}, 'withLocale' );Which fails compilation with: To me, it seems like this super basic use case should be supported without adding, e.g. |
|
@noahtallen I was able to make type LocaleProps = { locale: string };
export const withLocale = createHigherOrderComponent< LocaleProps >(
< TProps extends LocaleProps >( InnerComponent: React.ComponentType< TProps > ) => {
return ( props: Omit< TProps, keyof LocaleProps > ) => {
const locale = useLocale();
const innerProps = { locale, ...props } as TProps;
return <InnerComponent { ...innerProps } />;
};
},
'withLocale'
);Note that Gutenberg's The culprit is the same as described in #37795 (comment). TypeScript infers the Typing HOCs in TypeScript is incredibly painful, the two concepts are not very compatible. Everything's much simpler when everything is just hooks and functional components. |
|
...and here's an easier way to fix const innerProps = { locale, ...props } as React.ComponentProps<typeof InnerComponent>;
return <InnerComponent { ...innerProps } />; |
|
Thanks @jsnajdr for the sleuthing. I am hoping too that the newer
No need to clarify, but I'm curious what mismatch you are talking about. When working on this I did find some situations frustrating in that I thought I should be able to rely on the type inference but couldn't. If you have a better idea at the fundamental issues maybe that would help us find a better solution? |
It's just that when typing a HOC, things are never simple. You do something wrong, and suddenly you get a very abstract warning like: One needs to do some considerable mathematical thinking to unpack this. Because everything is hard, the types are rarely pleasant to work with, like having inference where one would expect it, or are just plain wrong. Consider this usage of type PropsOne = { name: string };
type PropsTwo = { count: number };
const Foo = ( { name }: PropsOne ) => <div>{ name }</div>;
const predicate = ( { count }: PropsTwo ) => count > 0;
const IfFoo = ifCondition( predicate )( Foo );There are types defined, typechecking passes successfully, but there is an obvious type mismatch: the When I tried to fix How to proceed from here? It's no longer fun at all. Also, I don't know what was the motivation for the changes you did in this PR, you merely wrote that "existing types in A HOC is a function that transforms one component type to another, and their props don't need to be related at all. The case where new props are injected and old ones are passed is merely a special case, although the most frequent one. It's like when the type of |
|
Thanks for clarifying. I'll work on this and see if we can't make those errors clearer. I don't know if this feels like an inherent mismatch though that would be better with hooks, unless the claim is that with hooks we just don't have to worry about wrapped props. It looks like you have pointed out a flaw in my assumptions in #37795 Noted in that PR I wasn't fully confident in the solution, but I did let it sit open for a few weeks to try and find review on it at the time. The motivation was some confusion around how to properly type the HOC. IIRC I was primarily considering the prop-injecting HOC pattern and wanted to warn people if they were trying to supply a prop that would ultimately be ignored. In Gutenberg I see counter-examples such as the color-providing HOC. So again I think you're right in pointing out that the types were more correct before. There may still be a way to revert to that while removing the additional type wrappers like |
Is there a specific example of a HOC that had troubles with the old types and got better with the new types? I'd like to understand the issues better. At this moment I feel that we want to revert this PR to the old state, but not completely? There are some use cases that this PR actually improved, and we want to do some kind of synthesis? |
|
Looking back on some issues with Calypso code what I believe was happening is that the type of Or put more minimally, we had the kind of problem of using this… interface ToBinary<T> {
(input: T): Binary;
}…when we want this instead… interface ToBinary {
<T>(input: T): Binary;
}
You can see in this patch where I was trying to do that, but added the unnecessary and inaccurate constraint that |
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.
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.
Status
Ready for final review ✓
There are a couple things I'm unhappy with about this refactor. I think it's more accurate
than the existing types, which I find confusing and think might be wrong. Still, it doesn't
handle components with
refparameters well (seewith-global-events) and I wouldprefer that we have full inference with fewer necessary type annotations.
Update: For now I want to ignore the
refissues andwith-global-eventssince thatcomponent has a number of more difficult type issues and it's marked as deprecated.
Let's not hold up this work to spend time describing a function we don't want people using.
Type annotations we don't want
Everything is passing the tests right now though so I don't want to totally hold off on this, but I'd like to let it stew for a bit and see if anyone has suggestions on how to close the gap. For reference, here is how I think `withInstanceId` should be able to be written (pay attention to the lack of type annotations - we shouldn't be losing any auto-complete or type safety).Description
The existing types in
createHigherOrderComponent<TOuter, TInner>are somewhatsurprising because they require defining what possible prop types are allowed
for components wrapped by the higher order component it produces. Additionally
it doesn't concern itself with preventing the wrapped component from accepting
or demanding props it itself provides.
In this patch we're reworking the types to only require a single type parameter,
which is the type of props that the higher order component injects into the
wrapped component. The prop types for the wrapped component are inferred only
when wrapping a component, and the injected props are removed from the external
interface to avoid confusion.
How has this been tested?
This is a type-only change that shouldn't impact the generated bundles.
Types of changes
Updating type signature for
createHigherOrderComponentChecklist:
*.native.jsfiles for terms that need renaming or removal).cc: @sirbrillig