Skip to content

Add type definitions for TypeScript (fixes #10) #12

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

Merged
merged 7 commits into from
Nov 23, 2018
Merged

Conversation

ghengeveld
Copy link
Member

No description provided.

@ghengeveld ghengeveld changed the title Add type definitions for TypeScript Add type definitions for TypeScript (fixes #10) Nov 21, 2018
@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #12 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #12   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          95     95           
  Branches       37     37           
=====================================
  Hits           95     95

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55d7b59...4e8ffa2. Read the comment docs.

Copy link

@tomshane tomshane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just few things.

  1. Regarding recent React changes and addition of hooks, it is recommended to use React.FunctionComponent<> instead of React.SFC<>. SFC type is going to be deprecated.

  2. To pass arguments to <Async> component, I can't use the way it is done in plain JS:
    <Async promiseFn={this.getSession} sessionId={123}/>

    Is this intended way how to pass arguments when using TS?:
    <Async promiseFn={this.getSession} {...{sessionId: 123}}/>

  3. It would be cool to have typed data object in callbacks via generics. I tried it when using my own typings and I was not able to make everything work as I would like. I will keep trying, but if anyone could come with a solution, it would be perfect.

@tomshane
Copy link

tomshane commented Nov 22, 2018

If you would like to have a look, here is my current typings file. On my current setup (TS 3.1.4), generics seem to work for base Async component. For helper components, I didn't find a way how to improve typing.

declare module "react-async"
{
  import * as React from "react";

  interface AsyncProps<T> {
    promiseFn?: (props: object) => Promise<T>;
    deferFn?: (...args, props: object) => Promise<T>;
    watch?: any;
    initialValue?: T;
    onResolve?: (data: T) => void;
    onError?: (error: Error) => void;
    children?: AsyncChildren<T>;
  }

  interface AsyncState<T> {
    initialValue: T,
    data: T,
    error: Error,
    isLoading: boolean,
    startedAt: Date,
    finishedAt: Date,
    cancel: () => void,
    run: (...args) => Promise<T>,
    reload: () => void,
    setData: (data: T, callback?: () => void) => T,
    setError: (error: Error, callback?: () => void) => Error
  }

  function createInstance<T>(defaultProps?: AsyncProps<T> = {}): Async<T>;

  type AsyncChildren<T> = ((state: AsyncState<T>) => React.ReactNode) | React.ReactNode;

  class Async<T> extends React.Component<AsyncProps<T>, AsyncState<T>> {
    static Pending: React.FunctionComponent<{ children?: AsyncChildren, persist?: boolean }>;
    static Loading: React.FunctionComponent<{ children?: AsyncChildren, initial?: boolean }>;
    static Resolved: React.FunctionComponent<{ children?: AsyncChildren, persist?: boolean }>;
    static Rejected: React.FunctionComponent<{ children?: AsyncChildren, persist?: boolean }>;
  }

  export default createInstance;
}

@ghengeveld
Copy link
Member Author

Looks good 👍 I'll update my PR.
Shouldn't most of the state values be optional?

interface AsyncState<T> {
  initialValue?: T
  data?: T
  error?: Error
  isLoading: boolean
  startedAt?: Date
  finishedAt?: Date
  cancel: () => void
  run: (...args) => Promise<T>
  reload: () => void
  setData: (data: T, callback?: () => void) => T
  setError: (error: Error, callback?: () => void) => Error
}

@tomshane
Copy link

Yes, your AsyncState is the correct one.

@ghengeveld
Copy link
Member Author

I've pushed the latest changes. VS Code was complaining about the deferFn?: (...args, props: object) because rest params aren't supported in that position by TypeScript, so I just removed the props param and have the ...args match it.

Also VS Code was complaining about the generic type T not being accessible to static members of Async. Maybe it works regardless?

Could you check if this works?

@tomshane
Copy link

tomshane commented Nov 23, 2018

I placed the index.d.ts file into node_modules/react-async folder and VS Code didn't complain at all. No errors in console while compiling either. So here, everything looks great.

@ghengeveld
Copy link
Member Author

Alright, I'm merging it then. We can always make tweaks later.

@ghengeveld ghengeveld merged commit a670f23 into master Nov 23, 2018
@ghengeveld ghengeveld deleted the typings branch November 23, 2018 17:14
@ghengeveld
Copy link
Member Author

ghengeveld commented Nov 23, 2018

Published in v3.8.2

@ghengeveld
Copy link
Member Author

Hey @tomshane, I've been looking at this again and I'm not happy with the way the static members are defined. Their type signature is obviously broken because a static member cannot access the parent class' generic type. I've been fiddling around with it all day but still have not found a solution. So I had to ask: https://stackoverflow.com/questions/53564005/correct-type-definition-for-a-react-class-with-static-member-components

Maybe you can take a look at it?

@tomshane
Copy link

tomshane commented Dec 1, 2018

Yea, I was trying before, but I gave up, because I couldn't make it work. That's why I left the helper components non-generic in my draft. I have found this: microsoft/TypeScript#24018, so it seems it is not supported in the current TS.

@ghengeveld
Copy link
Member Author

Well they don't have to reference the class generic, they can have their own. I just haven't found a way to do that yet.

@tomshane
Copy link

tomshane commented Dec 1, 2018

I don't know if that's what you have in mind, but I've tried this:

declare class Async<T> extends React.Component<AsyncProps<T>, AsyncState<T>> {  
  static Pending: <T>(props: { children?: AsyncChildren<T>; persist?: boolean }) => JSX.Element;
  static Loading:  <T>(props: { children?: AsyncChildren<T>; initial?: boolean }) => JSX.Element;
  static Resolved: <T>(props: { children?: AsyncChildren<T>; persist?: boolean }) => JSX.Element;
  static Rejected: <T>(props: { children?: AsyncChildren<T>; persist?: boolean }) => JSX.Element;
}
export default Async;

This way I was able to use helper components without an issue. However, the generic type argument won't be inferred, so the usage must be either:

<Async.Resolved>
  {(data: MyData) => { ... }}
</Async.Resolved>

or:

<Async.Resolved<MyData>>
  {(data) => { ... }}
</Async.Resolved>

@ghengeveld
Copy link
Member Author

@all-contributors please add @tomshane for review

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @tomshane! 🎉

@ghengeveld
Copy link
Member Author

@all-contributors please add @ghengeveld for code, review, question, blog, doc, design, example, financial, infra, maintenance

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @ghengeveld! 🎉

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 this pull request may close these issues.

2 participants