mobxjs / mobx-react-lite

Lightweight React bindings for MobX based on React 16.8 and Hooks
https://mobx.js.org/react-integration.html
MIT License
2.13k stars 90 forks source link

Keep generic arguments when using `observer` #243

Closed Kukkimonsuta closed 4 years ago

Kukkimonsuta commented 4 years ago

When using observer to wrap component that accepts generic argument the generic argument is lost effectively removing option to have observer components with generics. I've been able to hack together following workaround, but it would be great if generics weren't lost by default in observer. mobx-react works with generic component correctly.

Workaround:

function genericObserver<T extends React.FunctionComponent<P>, P extends object = T extends React.FunctionComponent<infer P> ? P : unknown>(fn: T): T {
    return observer<P>(fn) as T;
}

Current behavior: image

Desired behavior (similar to mobx-react): image

Sample codesandbox: https://codesandbox.io/s/zen-mestorf-4hp7q

danielkcz commented 4 years ago

I am confused, looking at your CodeSandbox, how is the outcome different? In both cases, I see that value is unknown.

const Bar = observer(<T extends unknown>({ value }: { value: T }) => {
  return <>{JSON.stringify(value)}</>;
});

const Baz = genericObserver(<T extends unknown>({ value }: { value: T }) => {
  return <>{JSON.stringify(value)}</>;
});

mobx-react works with generic component correctly.

That is really strange thing to say because mobx-react@6 builds on top of this lib and doesn't do anything special about generics. On the contrary it works even worse. Try to switch for mobx-react in your sandbox, the <P> won't work anymore.

Kukkimonsuta commented 4 years ago

Within the component itself the typing is fine, however outside (after observer is applied) the generic parameter gets replaced by unknown and cannot be inferred.

image

vs

image

These screenshots are taken directly from the sandbox.

I suspect mobx-react works because the typing for observer is different:

export type IReactComponent<P = any> =
    | React.StatelessComponent<P>
    | React.ComponentClass<P>
    | React.ClassicComponentClass<P>

export function observer<T extends IReactComponent>(target: T): T

vs

export function observer<P extends object, TRef = {}>(
    baseComponent: React.RefForwardingComponent<TRef, P>,
    options: IObserverOptions & { forwardRef: true }
): React.MemoExoticComponent<
    React.ForwardRefExoticComponent<React.PropsWithoutRef<P> & React.RefAttributes<TRef>>
>
export function observer<P extends object>(
    baseComponent: React.FunctionComponent<P>,
    options?: IObserverOptions
): React.FunctionComponent<P>

For mobx-react the genericObserver workaround doesn't work, but it doesn't need to as observer returns generic function already:

image

danielkcz commented 4 years ago

Sorry, still confused. Why do want to inference for props to work? What's the point? On the contrary, you should be doing observer<{ value: number }>(({ value }) => ... so it's properly type checked whenever you used that component somewhere else.

Kukkimonsuta commented 4 years ago

I have chosen simple example where it doesn't really make sense, however once you bring in bit more complexity it can be very useful - if the type is inferred you can use it multiple times and type checker ensures the types are the same. This is very useful for passing data down to a component that are expected to come back in some form using a callback.

In the following sandbox there is simple select component that can accept value, options and onChange and get type check that all are of the same type, but not limiting consumer to using specific one. Note that if you change genericObserver to observer the sample stops working as all types become unknown.

https://codesandbox.io/s/frosty-bouman-yrge3

danielkcz commented 4 years ago

Thank you for a better example, now it makes more sense :) Would you mind sending in PR to fix that?

Kukkimonsuta commented 4 years ago

Sure, I'll see what I can do :)

danielkcz commented 4 years ago

Merged and published to v2.0.0-alpha.5

timothyallan commented 4 years ago

It's still not possible to use useImperativeHandle with this update, as even with forwardRef:true in observer. Parent components still report ref as being undefined. If I do something like const ImperativeComponent= observer(forwardRef(myComponent)); and export that, it works. But I think this causes issues with memo inside of observer glancing through the code.

jonasalexander commented 4 years ago

Hey, I was unable to get this to work with the changes in the linked PR here and have only been able to get it to work using @Kukkimonsuta's code snippet

function genericObserver<
  T extends React.FunctionComponent<P>,
  P extends object = T extends React.FunctionComponent<infer P> ? P : unknown
>(fn: T): T {
  return observer<P>(fn) as T;
}

What is the recommended way of having generic observers given the changes @Kukkimonsuta made? Thanks!

See a simple example here: https://stackoverflow.com/questions/63725232/mobx-generic-observer-using-typescript-typing

Kukkimonsuta commented 4 years ago

@jonasalexander I have answered your SO question, but for completeness let me put it in here as well:

// original
interface GenericProps<T> {
    arg: T;
}
const foo: <T>(props: GenericProps<T>) => T = (props) => props.arg;

// 1. your sample is not react component, you need to match `React.FunctionComponent` interface
const foo_1: <T>(props: GenericProps<T>) => React.ReactElement | null = (props) => <>{JSON.stringify(props.arg)}</>;

// 2. lets add observer (this should work already since TS 3.4 or 3.5)
const foo_2: <T>(props: GenericProps<T>) => React.ReactElement | null = observer((props) => <>{JSON.stringify(props.arg)}</>);

// 3. if on older TS you need to move type declaration to the function itself (note the `extends unknown` 
// which tells compiler this is generic definition and not JSX tag)
const foo_3 = observer(<T extends unknown>(props: GenericProps<T>) => <>{JSON.stringify(props.arg)}</>);