Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typescript types seem not to work properly #230

Closed
yannick-cw opened this issue Jul 23, 2019 · 9 comments · Fixed by #242
Closed

Typescript types seem not to work properly #230

yannick-cw opened this issue Jul 23, 2019 · 9 comments · Fixed by #242

Comments

@yannick-cw
Copy link

yannick-cw commented Jul 23, 2019

Hey, I tried to integrate react-refetch to our stack and could not get it to work with typescript properly. To me it looks like there are some problems with the type definitions, but maybe I missed something on usage side.

Here is a short example

import React from 'react';
import { connect, PromiseState } from 'react-refetch';

interface Props {
  exampleFetch: PromiseState<any>;
  exampleUpdate: (term: string) => void;
  exampleUpdateDone?: PromiseState<string>;
}

export class SmallExampleComp extends React.PureComponent<Props> {
  public render() {
    return <div>ok</div>;
  }
}

const ConnectedComp = connect<Props>(_props => ({
  exampleFetch: 'url',
  exampleUpdate: (_term: string) => ({
    exampleUpdateDone: {
      url: 'url',
    },
  }),
}))(SmallExampleComp);

export const Usage = <ConnectedComp />;

This fails to compile at two places
First at usage side it seems to still require the properties, that are actually added in the connect:

Type '{}' is missing the following properties from type 'Readonly<Props>': exampleFetch, exampleUpdate

26 export const Usage = <ConnectedComp />;

Second the ? optional on the exampleUpdateDone does not work with the connect, but is required, as the exampleUpdateDone is optional.

    Types of property 'exampleUpdateDone' are incompatible.
      Type '{ url: string; }' is not assignable to type 'undefined'.

19   exampleUpdate: (_term: string) => ({

If I am missing something please give me a hint, otherwise I'd like to help fix these type definitions.

@yannick-cw
Copy link
Author

Okay I have fixed the types for us locally with this version:

import { ComponentClass, FunctionComponent } from 'react';
import { connect as badConnect, Mapping, WithRefetch } from 'react-refetch';

type FixedConnect = <FetchProps, CompProps>(
  map: MapPropsToRequestsToProps<FetchProps, CompProps>,
) => (
  component: ComponentClass<CompProps> | FunctionComponent<CompProps>,
) => ComponentClass<Omit<CompProps, keyof FetchProps>> & WithRefetch<Omit<CompProps, keyof FetchProps>>;

type MapPropsToRequestsToProps<FProps, CProps> = (props: Omit<CProps, keyof FProps>, context: any) => PropsMap<FProps>;

type PropsMap<FProps> = {
  [FProp in keyof FProps]: PromiseStateMapping<FProps> | FunctionMapping<FProps, FProp>;
};

type PropsMapInner<FProps> = {
  [FProp in keyof FProps]?: PromiseStateMapping<FProps> | FunctionMapping<FProps, FProp>;
};

type PromiseStateMapping<FProps> = string | Mapping<FProps, any>;

type FunctionMapping<FProps, FProp extends keyof FProps> = FProps[FProp] extends ((...args: infer FArgs) => void)
  ? ((...args: FArgs) => PropsMapInner<FProps>)
  : never;

export const connect: FixedConnect = badConnect as FixedConnect;

It does not have defaults and options of connect, as we did not need it yet.

@ryanbrainard
Copy link
Contributor

Sorry for the VERY long delay on responding to this.

@lmontoute would you happen to have any thoughts on this issue? Perhaps it would be helpful to add a quick example to the readme using TypeScript. I was experimenting with it myself today and ran into some snags with how the props should be passed defined and passed in, so I'm sure it would be helpful to others too.

cc'ing a couple more folks from #219 who might like to contribute a sample to docs: @rjhilgefort, @hburrows, @ipanasenko

@ryanbrainard
Copy link
Contributor

ryanbrainard commented Oct 28, 2019

Specifically, the problem I ran into was a bit different than described above, but it seems to be same root cause. For me, the problem is that MapPropsToRequestsToProps is expecting the props to be the same both for the input and output, so you end up having to put the *Fetch mapping prop in the actual component's props as required. This works fine for the component itself, but not when you actually call it from elsewhere.

Here is a simplified example:

import React, { Component } from 'react'
import { connect, PromiseState } from 'react-refetch'
import * as types from '../types'

interface Props {
  executionId: string
  executionFetch: PromiseState<types.WorkflowExecution>
}

class WorkflowState extends Component<Props> {
  render() {
    // do something with this.props.executionFetch
    return null
  }
}

export default connect((props: Props) => {
  return {
    executionFetch: `/executions/${props.executionId}`,
  }
})(WorkflowState)

Notice how executionFetch is required. Changing it to executionFetch?: PromiseState<types.WorkflowExecution> fails with:

Error:(17, 24) TS2345: Argument of type '(props: Props) => { executionFetch: string; }' is not assignable to parameter of type 'MapPropsToRequestsToProps<Props>'.
  Type '{ executionFetch: string; }' is not assignable to type 'PropsMap<Props>'.
    Types of property 'executionFetch' are incompatible.
      Type 'string' is not assignable to type 'undefined'.

Keeping executionFetch required (as originally shown above), it compiles, but then trying to actually use the component fails because executionFetch is not set:

Error:(49, 14) TS2769: No overload matches this call.
  Overload 1 of 2, '(props: Props, context?: any): Component<Props, any, any>', gave the following error.
    Property 'executionFetch' is missing in type '{ executionId: string; }' but required in type 'Readonly<Props>'.
  Overload 2 of 2, '(props: Props, context?: any): Component<Props, any, any> & WithRefetchInstance<Props>', gave the following error.
    Property 'executionFetch' is missing in type '{ executionId: string; }' but required in type 'Readonly<Props>'.

It looks like @yannick-cw is on to something with his fix, but I'm curious how others are using the TypeScript definitions because it seems like you'd quickly run into these problems. Of course, I'm a TypeScript noob and could completely be doing it wrong 🤷‍♂

@ddanielbee
Copy link

ddanielbee commented Nov 11, 2019

The problem is still present in v3.0. To update in the same stack @yannick-cw mentioned, we did the following:

type MapPropsToRequestsToProps<FProps, CProps> = (props: Omit<CProps, keyof FProps>) => PropsMap<FProps>;

The only thing different is the removal of the context parameter.

This keeps everything working so far without any other changes to our codebase, so his original fix is still compatible after the update.

@ryanbrainard
Copy link
Contributor

@yannick-cw 's fix above has been integrated in #242. PTAL :)

@yannick-cw
Copy link
Author

Nice, thanks @ryanbrainard

@ryanbrainard
Copy link
Contributor

Oh, the PR (#242) isn't merged yet, so re-opening this. @yannick-cw, does the example in the PR work how you were envisioning this be used?

@ryanbrainard ryanbrainard reopened this Dec 2, 2019
@yannick-cw
Copy link
Author

Oh I misread that, I'll take a look

@yannick-cw
Copy link
Author

@ryanbrainard I think the example looks good 👍

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

Successfully merging a pull request may close this issue.

3 participants