Fix losing props for observer component

[vlazh]

Some code in my project has broken after latest updates because resulting observer component type loose original props. Take a look at example.

Correct type checking:

Incorrect type checking (and children prop has appered):

Replace code:

And type checking is correct:

[danielkcz]

Your latest changes are not good, have a look at failing tests.

Besides, I've seen patterns like these that wouldn't be possible with your simplified approach. Let's not be to wild with changes, I don't want to make V3 so soon just because of TS changes.

const comp = observer<TProps>((props) => {...}) const comp = observer<TProps, TRef>((props, ref) => {...}, { forwardRef: true })

[vlazh]

Got it. Now just fixes props.

[danielkcz]

FIY the test is still failing ;) I don't think you can make those typings much smaller really. Try to follow ideas from Michel on how to improve what you had.

[vlazh]

Seems like my first edition was right.

I think this should just read C, which is already a React.FunctionComponent

It is error with that modification. And other suggestions are not helped.

[vlazh]

That's it! Tests are passed.

[danielkcz]

Ok, and can you please add a test that covers your use case? To avoid regressions in the future.

[vlazh]

Yes sure!

[vlazh]

Done.

[danielkcz]

Thanks, published 2.0.4.

[seivan]

Sorry but, unless I'm missing a bigger picture here, this causes a regression by adding implicit children which is not right. This means all your components are now expecting children, and you won't get a compile error if you're adding children to a component that isn't expecting it.

Playground

The previous implementation would give you a compile error if you inserted children when they weren't expected which is more correct and this just removes more checks.

[danielkcz]

@seivan These types are already complicated as is. If you have some idea on how to cover a problem of this issue and yours, PR would be most welcome. This is too advanced for me.

[seivan]

@mweststrate I think you're on to something with this comment just using C will suffice.

const A = ({ a }: AProps) => {
    return <div>{a}</div>;
}
//Inferred Type Signature
//const ObserverA: ({ a }: AProps) => JSX.Element
const ObserverA = observer(A);

const B = ({ a, children }: React.PropsWithChildren<AProps>) => {
    return <div><h1>{a}</h1>{children}</div>;
}
//Inferred Type Signature
//const ObserverB: ({ a, children }: React.PropsWithChildren<AProps>) => JSX.Element
const ObserverB = observer(B);

[seivan]

@FredyC I am suggesting to minimize them by removing the complexity.

Replacing

C extends React.FunctionComponent<infer P> ? C & React.FunctionComponent<P> : never;

with

    : C

as suggested by @mweststrate in comment

But the reason I am writing here, is because I am not 100% sure if I understood the PR correctly and what it's actually solving. The impression I have is that @vlazh got his/her compile errors when passing children in on components that didn't have an explicit children?: React.ReactNode in their props.

[danielkcz]

The test @vlazh added should be fairly explanatory. I tried it without this change and this line would fail because a property is not present on the implicit Attributes (or something).

React.createElement(ObserverTestComponent, { a: 1 });

The best course of action would be if you can create a PR with your suggested change and add a test that covers your issue. Unless all tests are passing, we don't have a solution.

[seivan]

@FredyC That's odd, my tests passed.

What typescript version are you running?

[seivan]

@FredyC Made a PR without changing the tests. The tests ran successful locally with with TS 3.8.3.

@vlazh Would you mind testing the PR with whatever codebase you had issues with?

[vlazh]

I agree that adding props implicitly is not good. I have no problem just with

: C

while it keep all original props.

Consider to use React.NamedExoticComponent instead of {displayName: string}.

C extends React.FunctionComponent<infer P> ? C & React.NamedExoticComponent<P> : never;

It might worth to change tests.

[seivan]

Here's the thing, Typescript lets you declare attributes instead of a named interfaces to conform to as well.

Instead of saying C extends NamedExoticComponent and having to update that when that also gets deprecated and renamed for whatever reason, I can instead say C has {displayName?: string} and have it work none the less.

This is instead of introducing more meaningless types, until React decides to rename displayName, but at the very least, it's one less name to keep track of which is used far more than NamedExoticComponent.

[vlazh]

Using of {displayName: string} looks like dirty hack specially for tests. I prefer to use existing types if possible instead of introduce yet another type. It's more suitable type which returnd from React.memo which uses in observer. Tomorrow, React might decides to extend the NamedExoticComponent and in that case we need nothing to do.

[seivan]

Now, I could be wrong but as far as React is concerned, only displayName exists, in fact, NamedExoticComponent is some foreign interface stored on DefinitelyTyped.

It's worth taking a step back before calling stuff hacks, because the reason why you had issues and these messes happen is because code like:

 ? C extends React.RefForwardingComponent<infer TRef, infer P>
        ? C &
              React.MemoExoticComponent<
                  React.ForwardRefExoticComponent<
                      React.PropsWithoutRef<P> & React.RefAttributes<TRef>
                  >
              >
        : never /* forwardRef set for a non forwarding component */
    : C extends React.FunctionComponent<infer P> ? C & React.FunctionComponent<P> : never;

There's nothing inherently wrong with it, it's just... that it exists.

Now, we need JUST displayName, do we introduce NamedExoticComponent or do we outright say, that that displayName is part of the type?

Think about this We don't care that X conforms to NamedExoticComponent We care that X has {displayName: string}, because that's what we're calling when we need the displayName

A hack is introducing 30 different granular interfaces because of some pipe dream about good abstractions.

[vlazh]

We care that X has {displayName: string}, because that's what we're calling when we need the displayName

Yes, and we already have specially type from react types lib for that case but you prefer write your own. May be it's absolutly no matter for you but

I prefer to use existing types if possible instead of introduce yet another type.

And relying on source code of observer

It's more suitable type which returnd from React.memo which uses in observer.

I don’t understand what you are arguing about. :) In you PR you propose your solution for which I propose you to use a little bit another approach (better for my opinion) and you decide which you want use. Eventually last word for maintainers.