fix: names of mapped props for shorthand factories

[kuzhelov]

Problem

The following method is currently used to produce shorthand factory:

Button.create = createShorthandFactory(Button, 'content')

The second argument provided to it was treated as object of general string type - thus the following expression would be seen as absolutely valid by TS compiler:

Button.create = createShorthandFactory(Button, 'ahaha')

This fact has caused problems for several components where prop name has pointed to (already) non-existent prop - these are fixed in this PR.

Prevention actions

To prevent this issue from happening again there were additional type checks introduced - those should lead to compile error in case if provided prop name would refer to non-existent prop of the component.

Note that all necessary type checks were also introduced for shorthand tests factory methods.

[layershifter]

This type looks like a pure evil 😈 We had a similar issue before and I solved it with a generic type:

export function createShorthandFactory<P>(
  Component: React.ReactType<P>,
  mappedProp?: keyof P & string, // `string` may be redundant there
) {

Yes, we should update all files, but this type check is much simpler and obvious.

[kuzhelov]

@layershifter

for the suggested approach

• string type should be definitely eliminated from the intersection type in either case, because, otherwise, it will just match all the cases we were struggling before
• for the rest of the suggested part - unfortunately, type inference for the expression you are suggesting won't take place - because compiler will be pretty happy to use any as type for P and this will satisfy all the conditions of the typing expressions used in the example. To really utilize type inference we have no other options than use infer in type declarations.

I don't think that this type looks unreadable - actually, am pretty sure that any infer expression suggested in either TS guide or Deep Dive book would look similar.

At the same time, I've intentionally made these changes to be unobtrusive - you might notice that I've affected public interfaces of the methods only, without touching the actual types that were used before inside of the methods. Have done this intentionally, to be sure that this type guard functionality could be easily modified/fixed/removed if necessary.

[kuzhelov]

yes, this is, actually, from the TS docs: https://github.com/Microsoft/TypeScript/wiki/What's-new-in-TypeScript#generic-rest-parameters

type Unpacked<T> =
    T extends (infer U)[] ? U :
    T extends (...args: any[]) => infer U ? U :
    T extends Promise<infer U> ? U :
    T;

don't get me wrong - I would be glad to use simpler expression to achieve the same goal (and opened to any suggestions), just now see it as the only (reliable) solution available to us

[kuzhelov]

oh, and now, probably, see another reason for your point (apart from the general one) - thing is that I've used another formatting initially:

T extends React.ComponentClass<Extendable<infer TProps>> ? (ChildrenProps & TProps) 
  : T extends React.ComponentClass<infer TProps> ? (ChildrenProps & TProps) 
  : T extends React.StatelessComponent<Extendable<infer TProps>> ? (ChildrenProps & TProps) 
  : T extends React.StatelessComponent<infer TProps> ? (ChildrenProps & TProps)
  : any

This was easier to read - but now, with the prettier changes it becomes absolutely cryptic. Will try to address this readability issue

[mnajdova]

I've tested this myself locally too. @layershifter your proposal works, but as you mention we will need to update all createShorthandFactory usages, if we want to leverage this. If we forget to do this, we may still have the previous errors. @kuzhelov 's suggested typings don't require this from the user, and offer out of the box checking for this. I see this as a better alternative and big improvement step. I vote for the typings implemented in the PR.

[layershifter]

@mnajdova I agree and fully support improvals there, but how much time do you need to understand what is happening in there? To be honest, can you say that this type is a simple and obvious?

[mnajdova]

I am not saying it is simple and obvious, but I don't think that just because it is not simple, we should not add it. Improving our skills and knowledge will mean we will start adding more complicated typings, and I am totally supporting that. @kuzhelov maybe just short description above the type will help others to better understand it.

[miroslavstastny]

This is insane! 👍

[kuzhelov]

have tried alternative options, but all of them require type inference and, thus, the same amount of infer and related ternary operators. Lets leave this approach for now, as we have functionality that is necessary for us and it will be encapsulated under meaningful name. We can return to that and refactor later if necessary

[layershifter]

The described issue is valid and should be resolved, however I prefer less magic solution. We can suddenly meet issues like in #550.

[kuzhelov]

@layershifter,

I have intentionally made this changes to touch public API only, so it would be easier to

• fix typings
• suggest new approach to these guard typings
• remove them if necessary (in case if signature of factory method will change)

Thus, I think that as long as these imperative part you are worrying about

• is not scattered all around the code
• can be easily fixed or removed

as long as this is the case, we are safe to move forward with that

[mnajdova]

👍

[mnajdova]

👍 for adding the false value as a option!

[miroslavstastny]

This is insane! 👍