Preserve generics when using observer

[Kukkimonsuta]

Related issue #243

This change allows wrapping generic components while preserving generic parameters.

There is also unintended change that preserves all static types. This has mostly positive effect, though $$typeof, type, compare, and render (which are not copied over) remain with their original type if defined on base component. I didn't find a way to omit these properties as any kind of narrowing will cause generic parameters to be converted to unknown.

[danielkcz]

@xaviergonz @mweststrate Could really use your help with reviewing this, I am not that strong with TypeScript yet.

[mweststrate]

I'll try to take a look tomorrow.

…

On Mon, Nov 25, 2019 at 9:22 AM Daniel K. ***@***.***> wrote: @xaviergonz <https://github.com/xaviergonz> @mweststrate <https://github.com/mweststrate> Could really use your help with reviewing this, I am not that strong with TypeScript yet. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#244?email_source=notifications&email_token=AAN4NBF57HLDORTIEXOH4M3QVOKNFA5CNFSM4JRFGI42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFBWLTY#issuecomment-558065103>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBCLM6NSDEZXI6TFME3QVOKNFANCNFSM4JRFGI4Q> .

[Kukkimonsuta]

I have noticed an issue where empty options were not accepted, which while silly could be an issue in future if more options are added. Should be fixed in last commit.

[mweststrate]

The types are getting fairly complicated / overloaded now, I wonder if we can make it a bit simpler by avoiding overloads, and inferring return type roughly as follows, I think the type inference and error messages will be better if we do it this way, as it avoids 'backtracking' the generic arguments, and takes the input argument as starting point:

export function observer<
  T extends React.FunctionComponent<any> | React.RefForwardingComponent<any>, 
  Options extends IObserverOptions
>(
    baseComponent: C,
    options?: Options,
): Options extends { forwardRef: true } 
  ? C extends React.RefForwardingComponent<infer TRef, infer P> 
    ? React.ForwardRefExoticComponent<React.PropsWithoutRef<P> & React.RefAttributes<TRef>>
    : never /* forwardRef set for a non forwarding component */
  : C

[danielkcz]

As Michel said...

I wonder if we can make it a bit simpler by avoiding overloads

@Kukkimonsuta Did you try to remove other overloads?

[Kukkimonsuta]

I have merged the two new overloads using Michels sample with slight modifications, however I'm not certain whether we want to remove the original overloads too as it would be breaking change and following transformation in users code would have to be done by users during migration to new version:

interface IProps {
    value: string;
}
const TestComponent = observer<IProps>(({ value, children }) => {
    return null
})

to

interface IProps {
    value: string;
}
const TestComponent = observer(({ value, children }: React.PropsWithChildren<IProps>) => {
    return null
})

[mweststrate]

Hmmm, I've never seen the first pattern in the wild, personally. I think the typical thing to do is to add `children: React.ReactNode` to the `IProps`, especially because they don't have to be necessarily react nodes. Personally I'd release it as minor, and rollback or add the additional overload if it is an obstacle for many.

…

On Wed, Dec 18, 2019 at 5:14 PM Lukáš Novotný ***@***.***> wrote: I have merged the two new overloads using Michels sample with slight modifications, however I'm not certain whether we want to remove the original overloads too as it would be breaking change and following transformation in users code would have to be done by users during migration to new version: interface IProps { value: string; }const TestComponent = observer<IProps>(({ value, children }) => { return null }) to interface IProps { value: string; }const TestComponent = observer(({ value, children }: React.PropsWithChildren<IProps>) => { return null }) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#244?email_source=notifications&email_token=AAN4NBB6AO2YGXXGRMDB6STQZJK65A5CNFSM4JRFGI42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHG2U6I#issuecomment-567126649>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBDK4GRUBDOTHKUEXS3QZJK65ANCNFSM4JRFGI4Q> .

[danielkcz]

Btw, this next branch, we can afford to have some breaking changes, there is very few of those so far. But I personally want to keep observer<Props> for sure.

[danielkcz]

I am usually doing this and we have various aliases for children (NoChildren, SingleChild, Required<SomeChildren>, etc...).

type TProps = SomeChildren & {
  extra: string
}

export const Comp = observer<TProps>(({ extra }) => { ... })

I suppose doing observer(({ extra }: TProps) => { ... }) is not too horrible, but it will be pain to refactor that. We should probably create a codemod or something to migrate this automatically.

The "problem" with official PropsWithChildren is it refers to ReactNode which includes undefined and that is wrong (an app will crash). It's some legacy crap going deep into how class components work. I have a custom ReactNode as using only FC and it works nicely.

[mweststrate]

ok, maybe this pattern is more common than anticipated, and we should continue support it, if 2/3 of the people in this thread already use it :-P. It's pretty as well. I don't think we have it somewhere documented explicitly, don't we?

…

On Wed, Dec 18, 2019 at 7:33 PM Daniel K. ***@***.***> wrote: I am usually doing this and we have various aliases for children ( NoChildren, SingleChild, Required<SomeChildren>, etc...). type TProps = SomeChildren & { extra: string } export const Comp = observer<TProps>(({ extra }) => { ... }) I suppose doing observer(({ extra }: TProps) => { ... }) is not too horrible, but it will be pain to refactor that. We should probably create a codemod <https://github.com/tusharmath/ts-codemod> or something to migrate this automatically. The "problem" with official PropsWithChildren is it refers to ReactNode which includes undefined and that is wrong (an app will crash). It's some legacy crap going deep into how class components work. I have a custom ReactNode as using only FC and it works nicely. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#244?email_source=notifications&email_token=AAN4NBDXPECZXFNGQH77UYLQZJ3GZA5CNFSM4JRFGI42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHHHERI#issuecomment-567177797>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBFXDLWMRT3R4EPGU5LQZJ3GZANCNFSM4JRFGI4Q> .

[danielkcz]

It actually is documented ... https://mobx-react.js.org/observer-hoc
No surprise there since I wrote that docs 😎

[mweststrate]

Hahaha cool!

…

On Wed, Dec 18, 2019 at 8:01 PM Daniel K. ***@***.***> wrote: It actually is documented ... https://mobx-react.js.org/observer-hoc No surprise there since I wrote that docs 😎 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#244?email_source=notifications&email_token=AAN4NBFGGWQ2YXFWAHAUDZ3QZJ6SHA5CNFSM4JRFGI42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHHKDZQ#issuecomment-567189990>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN4NBESKUMTG22I4NV5WETQZJ6SHANCNFSM4JRFGI4Q> .

[Kukkimonsuta]

I've experimented a bit further with this, however at this point I think it's out of my ability to merge rest of the overloads into one declaration.

The main issue I see is that there is a conflict of typing authority between the two: observer<MyProps>((props) => null) needs observer to declare type of the function component passed in, however observer(<T extends unknown>(props: MyProps<T>) => null) relies on observer respecting whatever is passed in.

[mweststrate]

Sorry for the late follow up! Completely dropped the ball here, but I think it is fine as it is!

[danielkcz]

Ok, since this is going to next branch we can afford bit experimenting. I will publish another prerelease later with this.