compose: Add types to createHigherOrderComponent, ifCondition and pure
#30881
Conversation
sarayourfriend
added
[Type] Code Quality
sarayourfriend
changed the title
createHigherOrderComponent and ifCondition
|
Size Change: -75.2 kB (-5%) ✅ Total Size: 1.39 MB
ℹ️ View Unchanged
|
Did we confirm that Gutenberg Mobile works with TS files? |
|
What do you mean @gziolo? It looks like the mobile React native e2e tests worked fine, do we need confirmation beyond that this is safe? |
|
There is also https://github.com/wordpress-mobile/gutenberg-mobile that consumes the source code of WordPress packages. We need to ensure it understands TypeScript. As far as I can tell it doesn't use the same Babel config that Gutenberg uses: Well, we can merge this PR, but as an additional step, we will need to help the mobile team to enable TS support in https://github.com/wordpress-mobile/gutenberg-mobile if necessary. |
|
As long as |
They don't do the regular npm consuming 😅 Every production package declares its entry point for React Native, e.g.: They use sources so we need to ensure that their setup is able to work with TypeScript files. It shouldn't be that much work as I can read at: |
🤦♀️ of course, I completely forgot about this. Obviously I'm fully on board with helping them get TypeScript support anyway. What's the best way to do this, let it break and fix it later? I'm not sure how to tell how/what way it will break otherwise, but that doesn't feel good to me. |
|
I asked the mobile team in their channel on WordPress Slack to help here (link requires registration at https://make.wordpress.org/chat): |
sarayourfriend
force-pushed
the
add/types-create-higher-order-component
branch
from
4b81b46 to
a08b27b
Compare
sarayourfriend
requested review from
cameronvoell,
ellatrix,
guarani,
nerrad and
oandregal
as code owners
|
This is what I got from @oguzkocer:
We can land this PR as soon as CI turns green. |
@gziolo Did you maybe mean to ping someone else? I can't think of a time I dealt with ^, but maybe my memory is failing me. |
|
It would be nice to maybe have a companion Gutenberg Mobile PR to run some extra CI checks. |
sarayourfriend
force-pushed
the
add/types-create-higher-order-component
branch
from
a08b27b to
71923a0
Compare
sarayourfriend
changed the title
createHigherOrderComponent and ifConditioncreateHigherOrderComponent, ifCondition and pure
Thanks @ceyhun, what would be the best way to do this? |
|
You can open PR similar to what I had in the past: wordpress-mobile/gutenberg-mobile#3249. You only need to point the Gutenberg git submodule to the hash from this branch. |
|
@sarayourfriend @gziolo I actually just finished opening said PR in wordpress-mobile/gutenberg-mobile#3403. We'll see what the results are. 😄 |
|
Looks like the GB mobile checks are all green 😁 |
|
@dcalhoun, thank you for help 💯 |
| >( | ||
| mapComponentToEnhancedComponent: ( | ||
| OriginalComponent: ComponentType< TProps > | ||
| ) => ComponentType< Subtract< TProps, TObviatedProps > >, |
There was a problem hiding this comment.
Fun fact: The type of mapComponentToEnhancedComponent is HigherOrderComponent<TObviatedProps>.
There was a problem hiding this comment.
While that's true I don't think we can use the type here becuase we can't pass TProps down, as the HOC type only accepts it on the call signature and we need to grab it from above. But it is a fun fact 😁
There was a problem hiding this comment.
OK, I hoped for a little bit more than a fun fact but yes, now I see that HigherOrderComponent doesn't specify all the generic types we need 🙂
| * @return Component class with generated display name assigned. | ||
| */ | ||
| function createHigherOrderComponent< | ||
| TObviatedProps extends object, |
There was a problem hiding this comment.
Could this be called TRemovedProps? I had to look up the word "obviated" 🙂
There was a problem hiding this comment.
Yup! Removed is way simpler, I just didn't think of it before 😅
There was a problem hiding this comment.
One more idea: making the naming closer to the programmer's thinking, these props are TInjectedProps. The HOC injects them into the inner component. The fact that they are removed from the outer component's type is secondary.
|
The type HigherOrderComponent< TInnerProps, TOuterProps > = (
Inner: ComponentType< TInnerProps >
) => ComponentType< TOuterProps >;
function createHigherOrderComponent< TInnerProps, TOuterProps >(
mapComponent: HigherOrderComponent< TInnerProps, TOuterProps >,
modifierName: string
): HigherOrderComponent< TInnerProps, TOuterProps > {
return ( Inner ) => {
const Outer = mapComponent( Inner );
const displayName = Inner.displayName || Inner.name || 'Component';
Outer.displayName = `${ modifierName }(${ displayName })`;
return Outer;
};
}What do you think? Isn't this a better type? I can then define, e.g., the const withI18n = createHigherOrderComponent( function < TProps >(
Inner: ComponentType< TProps & I18nContextProps >
) {
return ( props: TProps ) => {
const i18nProps = useI18n();
return <Inner { ...props } { ...i18nProps } />;
};
},
'WithI18n' );The type annotations are quite minimal, I don't need to use any In an example component that's wrapped with I was unable to make |
|
There are some obstacles that prevent the new types to be used by
|
Indeed, this PR only partially types |
Yes! It's much simpler, I seem to have over complicated things 🙂 |
|
So actually there is a problem with the types you suggested. How would you fully type annotate the |
@sarayourfriend This point is still important so that the partial typechecking is run as expected:
|
|
@sarayourfriend I was able to type const ifCondition = < TProps, >(
predicate: ( props: TProps ) => boolean
): HigherOrderComponent< TProps, TProps > =>
createHigherOrderComponent(
( WrappedComponent ) => ( props ) => {
if ( ! predicate( props ) ) {
return null;
}
return <WrappedComponent { ...props } />;
},
'ifCondition'
);And types for const pure = createHigherOrderComponent(
< TProps, >( Wrapped: ComponentType< TProps > ) => {
return class extends Component< TProps > {
shouldComponentUpdate( nextProps: TProps ) {
return ! isShallowEqual( nextProps, this.props );
}
render() {
return <Wrapped { ...this.props } />;
}
};
},
'pure'
);There are some differences between these two. The return type of But for const pure: WhatTheType = createHigherOrderComponent( ... )What should I write in place of
But can it be written in terms of |
This is a major problem for type extraction. We need to be able to either explicitly annotate the return type in TS or as a JSDoc |
|
With the type I18nProps = {
__: ( s: string ) => string;
};
const i18nProps: I18nProps = {
__: ( s ) => s
};
const mapI18n = < TProps, >( Inner: ComponentType< TProps & I18nProps > ) => (
props: TProps
) => <Inner { ...props } { ...i18nProps } />;
const withI18n = createHigherOrderComponent( mapI18n, 'WithI18n' );Here the return type of And I can't pass the |
Well the type can be written as <TProps>(Inner: ComponentType<TProps>) => ComponentType<TProps>But can it be written in terms of the |
|
I think it's fine to write it explicitly as the inferred type shows. Alternatively we could add aliases for these types: export type HigherOrderComponent< TInnerProps, TOuterProps > = (
Inner: ComponentType< TInnerProps >
) => ComponentType< TOuterProps >;
export type SimpleHigherOrderComponent = < TProps >(
Inner: ComponentType< TProps >
) => ComponentType< TProps >;
export type PropRemovingHigherOrderComponent< TRemovedProps > = < TProps >(
Inner: ComponentType< TProps & TRemovedProps >
) => ComponentType< TProps >;As always waves hands names are hard. |
|
I've tried moving forward with this PR but @jsnajdr I think I've run into another snag. Please not the comment at the end of this playground link: |
|
@sarayourfriend The trouble is this definition of type InstanceIdProps = { instanceId: string | number };
const withInstanceId: PropRemovingHOC< InstanceIdProps > = createHigherOrderComponent(
< TProps >( Wrapped: ComponentType< TProps & InstanceIdProps > ) => {
return ( props: TProps ) => {
const instanceId = useInstanceId( Wrapped );
return <Wrapped { ...props } instanceId={ instanceId } />;
};
}
);Now when calling We hoped that I was able to fix it with this implementation: const withInstanceId = createHigherOrderComponent(
< TProps extends InstanceIdProps >( Wrapped: ComponentType< TProps > ) => {
return ( props: Omit< TProps, keyof InstanceIdProps > ) => {
const instanceId = useInstanceId( WrappedComponent );
return <Wrapped { ...props } instanceId={ instanceId } />;
};
}
);It explicitly omits React Redux They do a lot of additional magic with the |
|
Sweet. In that case can we move forward with this PR in its current state @jsnajdr? |
|
|
||
| export type SimpleHigherOrderComponent = < TProps >( | ||
| Inner: ComponentType< TProps > | ||
| ) => ComponentType< TProps >; |
There was a problem hiding this comment.
I wish this type could have a better name (what exactly is simple about it?) or if we could avoid it.
If HigherOrderComponent defaulted its second argument:
HigherOrderComponent< TInnerProps, TOuterProps = TInnerProps >then we could write just
HigherOrderComponent< TInnerProps >to declare a "simple" HOC.
There was a problem hiding this comment.
I'll do this in the next PR. I agree I hate the names I chose!
There was a problem hiding this comment.
OK and thanks for working on this. React HOCs are a really complex TypeScript puzzle. The types for React Redux connect are almost 700 lines of hardcore TypeScript. I wish we used other patterns (render props, hooks) where types are more straighforward.
Let's do it 🚀 |
Description
Adds types to
createHigherOrderComponentandifCondition. I converted both to TypeScript as it simplified the typing process greatly for these highly abstract functions.Part of #18838
How has this been tested?
Type checks pass. No runtime changes.
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).