Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

Keep generic arguments when using observer #243

Closed
Kukkimonsuta opened this issue Nov 24, 2019 · 10 comments
Closed

Keep generic arguments when using observer #243

Kukkimonsuta opened this issue Nov 24, 2019 · 10 comments

Comments

@Kukkimonsuta
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

danielkcz commented Nov 24, 2019

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
Copy link
Contributor Author

Kukkimonsuta commented Nov 24, 2019

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
Copy link
Collaborator

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

@Kukkimonsuta
Copy link
Contributor Author

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

@danielkcz
Copy link
Collaborator

Merged and published to v2.0.0-alpha.5

@timothyallan
Copy link

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
Copy link

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
Copy link
Contributor Author

@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)}</>);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants