contiamo / restful-react

A consistent, declarative way of interacting with RESTful backends, featuring code-generation from Swagger and OpenAPI specs 🔥
MIT License
1.87k stars 109 forks source link

Failed type checking on data object #191

Closed vitorverasm closed 4 years ago

vitorverasm commented 4 years ago

Describe the bug Hi everyone, love this lib ❤️ , but yesterday I got some inconsistencies.

So I generated an api.tsx with the CLI using my openapi.json file, everything worked fine. But when I was using the get request hook it showed that my data response type was TrendingResponse | null and vscode intellisense stoped working on data object because of the null union.

So I looked over Get.d.ts on node_modules/restful-react/lib/Get.d.ts and it was like that:

/**
 * State for the <Get /> component. These
 * are implementation details and should be
 * hidden from any consumers.
 */
export interface GetState<TData, TError> {
    data: TData | null;
    response: Response | null;
    error: GetDataError<TError> | null;
    loading: boolean;
}

As I removed the | null operator intellisense worked fine. So here is my question, am I missusing the lib or is there something wrong on my tsconfig or is a real bug?

To Reproduce I don't know if I'm missusing the lib so I'll just put some info about my types and hook generated.

// Main get hook
export const useTrendingByMediaTypeAndTimeWindowGET = ({
  media_type,
  time_window,
  ...props
}: UseTrendingByMediaTypeAndTimeWindowGETProps) =>
  useGet<TrendingResponse, void, void>(`/trending/${media_type}/${time_window}`, props);

export type UseTrendingByMediaTypeAndTimeWindowGETProps = Omit<
  UseGetProps<TrendingResponse, void>,
  'path'
> & {media_type: string; time_window: TimeWindowEnum};

export type TimeWindowEnum = 'day' | 'week';

export type TrendingResponse = {
  page?: number;
  total_pages?: number;
  total_results?: number;
  results?: MovieListResponseObject;
  status_message?: string;
  status_code?: number;
};

Expected behavior Show correctly autocompletion on vscode. When I remove the | null from Get.d.ts:

image

Screenshots image

image

Workspace info:

System:
    OS: macOS High Sierra 10.13.6
    CPU: (4) x64 Intel(R) Core(TM) i7-3520M CPU @ 2.90GHz
    Memory: 19.69 MB / 8.00 GB
    Shell: 5.3 - /bin/zsh
  Binaries:
    Node: 10.17.0 - /usr/local/opt/node@10/bin/node
    Yarn: 1.19.1 - /usr/local/bin/yarn
    npm: 6.11.3 - /usr/local/opt/node@10/bin/npm
    Watchman: 4.9.0 - /usr/local/bin/watchman
  SDKs:
    iOS SDK:
      Platforms: iOS 12.1, macOS 10.14, tvOS 12.1, watchOS 5.1
    Android SDK:
      API Levels: 23, 24, 26, 27, 28
      Build Tools: 23.0.1, 27.0.3, 28.0.3
      System Images: android-23 | Intel x86 Atom_64, android-23 | Google APIs Intel x86 Atom_64, android-24 | Intel x86 Atom_64, android-24 | Google APIs Intel x86 Atom_64, android-26 | Intel x86 Atom_64, android-26 | Google APIs Intel x86 Atom_64, android-28 | Google APIs Intel x86 Atom
  IDEs:
    Android Studio: 3.3 AI-182.5107.16.33.5264788
    Xcode: 10.1/10B61 - /usr/bin/xcodebuild
  npmPackages:
    react: 16.9.0 => 16.9.0
    react-native: 0.61.4 => 0.61.4
  npmGlobalPackages:
    react-native-cli: 2.0.1
fabien0102 commented 4 years ago

Hello @vitorverasm 👋

Can you add the related openapi specs (yaml), a classic mistake is to define:

TrendingResponse:
  type: object
  properties:
    page:
       type: number

instead of:

TrendingResponse:
  type: object
  properties:
    page:
       type: number
  required:
    - page

Every properties are nullable by default in openAPI so you need to specify the required field.

For you second point, the type is totally correct, data is well TData | null, this is because data is null when loading === true or if you have error. So you need to deal with the case (I know, it's annoying, but real life is about loading and error state also 😁)

fabien0102 commented 4 years ago

To have intellisense on data, you need to discriminate a bit:

const MyComp = () => {
  const {data} = useMyApi();

  if (!data) {
    return "no data"
  }

  return <h1>{data.title}</h1>
}

or with a fallback value

const MyComp = () => {
  const {data} = useMyApi();

  return <h1>{data ? data.title : "no title"}</h1>
}

Bref, you need to deal with the null case somehow to hint typescript.

vitorverasm commented 4 years ago

Yea I understood that nullable type, I have to deal with it and makes total sense. I used the null check operator and it worked:

return <Text>{data!.results}</Text>

But I'll check and show something if it comes null

About the API, I had the json file of the swagger format, so I had to translate to openapi 3.0, I think thats the problem with the types, because after I generated the data was returning void | null so I added this TrendingResponse type. But thats another issue and related to my setup because I don't have the openapi directly.

Thanks for the explanation I'll close this.

fabien0102 commented 4 years ago

Just for info, you don't need to translate your specs, our generator deal with everything (json/yaml, openapi 2 or 3) 😉

vitorverasm commented 4 years ago

Even swagger format 🤔 ? I know I'm extending this conversation but the CLI gave and error on this specs: https://api.stoplight.io/v1/versions/9WaNJfGpnnQ76opqe/export/oas.json

It gave me this error: Captura de Tela 2019-11-29 às 10 58 16