Missing type inference for controller.fetch about data-client HOT 20 CLOSED

I would recommend not adding a custom hook, but instead extending Controller, and passing it to CacheProvider. This will enhance your controller with whatever additional methods you want. (There are a bunch of downsides to creating 'function hooks')

from data-client.

Ah yes, I forgot to mention that this was done in the TodoList playground. Here's a fork with the example file: https://stackblitz.com/edit/coinbase-rest-hooks-cgd2d5?file=src/resources/UserResource.ts

It uses the tsconfig.json that's included with the TodoList example:

{
  "compilerOptions": {
    "outDir": "./dist",
    "baseUrl": "./src",
    "target": "esnext",
    "module": "esnext",
    "lib": ["dom", "esnext"],
    "jsx": "react-jsx",
    "declaration": true,
    "strict": true,
    "moduleResolution": "node",
    "allowSyntheticDefaultImports": true,
    "esModuleInterop": true,
    "experimentalDecorators": true,
    "emitDecoratorMetadata": true,
    "types": ["@anansi/webpack-config/types"],
    "paths": {
      "resources/*": ["resources/*"]
    },
    "noEmit": true
  },
  "include": ["src"],
  "exclude": ["node_modules", "dist"]
}

Here's a screenshot of the error:

from data-client.

Thanks for the report @tobobo !

This is actually intended behavior, but I'd love to hear what you think might improve things from a design perspective.

Currently

Controller.fetch returns exactly the same value as the endpoint you send. It is typed as such.

If you want the store value (normalized+denormalized), then you can get that explicitly after with controller.getResponse. See the example in https://resthooks.io/docs/api/Controller#fetch

await controller.fetch(PostResource.create, createPayload);
const denormalizedResponse = controller.getResponse(
  PostResource.create,
  createPayload,
  controller.getState(),
);

If you want the return value of your endpoint to be typed, an easy way to set it with RestEndpoint is by passing a process argument: https://resthooks.io/rest/api/RestEndpoint#typing

interface TodoInterface {
  title: string;
  completed: boolean;
}
const get = new RestEndpoint({
  path: '/',
  process(value): TodoInterface {
    return value;
  },
});

Future work

It should be possible to infer the possible legal values that can be normalized to a given schema. This can be used to set the return type correctly just based on the schema definition. This would mean you would at least have the expected properties from your definition. (But any class methods would obviously not exist)

What do you want?

I'm curious which one you're looking for! Do you need a class member maybe? Or just want more precision on types? I'm open to suggestions

from data-client.

Hi @ntucker, thank you! This illuminates a nuance of the behavior of fetch that I hadn't considered, and you're right that it's spelled out in the docs but I missed it. Oops!

I think the most intuitive solution would be to have a new method on controller that replaces fetch as the main imperative method for interacting with the store, so that the most common path taken by devs requiring imperative fetches results in a typed response. As it is I'll probably make my own useDenormalizedFetch hook which encapsulates the example given in your response and the Controller#fetch docs.

Thank you again for your response!

from data-client.

I am curious tho - do you care about a typed response, or getting the same value as in the store? The difference would be likely regarding any class methods or properties on your entity. Otherwise, you won't need to denormalize to get a typed response once some more inferences are put in place.

from data-client.

That's an interesting question and I'm reading through some more of the docs to make sure I grasp its implications. My short answer is that I think for my cases I'd want a method that does both!

In the cases I've looked at so far, I haven't cared about referential equality (neither for equality checking nor for avoiding re-rendering components or anything like that) and I haven't made use of any class methods on my Resources so far. As a result, I didn't even notice that my responses from fetch were missing class methods!

So, in spite of the warning on the fetch docs I assumed I was getting, for example, the same return value from fetch that I would get from a useSuspense hook for the same endpoint and arguments.

If it helps, I'm using fetch for a few different purposes currently—

• Performing mutating calls (POST, PUT) in response to user input (return value used)
• Performing non-mutating calls (GET) in response to user input when the result is not relevant to the state of a component (return value used)
• Re-fetching stale data from non-mutating calls (GET) (return value not used)

Sorry for a roundabout answer to the question, but since I'm still getting to know the library I thought giving more information about my use case and approach would be more helpful than providing a naive response to the question at face value :)

from data-client.

I ran in to a bit of a snag while trying to implement the possible workarounds.

Using a return type for process doesn't work because I'm explicitly setting the types for my endpoints so I can be exact about the arguments required (more exact than Partial<Entity>). This is done in the Github example as well, but typing an endpoint as (for example) MutateEndpoint loses the type inference on the endpoint options, so you no longer get the automatic return typing based on process. I could maybe fix this by typing my endpoint as some kind of RestTypeWithBody but at that point the type gymnastics required is much more complicated than just explicitly typing the controller.fetch return value.

Additionally, when trying to make a fetchDenormalized function on an extended Controller, I found that the entity I created wasn't actually showing up in the store and thus wasn't returned from getResponse. I must have something wrong in my configuration, as this is contrary to the tip in the fetch docs. However, I noticed that it doesn't work in the Todo List example either, when I replace the source of NewTodo.tsx with the following:

import { styled } from '@linaria/react';
import { useController } from '@rest-hooks/react';
import { useCallback, useRef, useState } from 'react';
import { TodoResource, Todo } from 'resources/TodoResource';

export default function NewTodo({ lastId }: { lastId: number }) {
  const ctrl = useController();
  const [title, setTitle] = useState('');
  const handleChange: React.ChangeEventHandler<HTMLInputElement> = useCallback(
    (e) => {
      setTitle(e.currentTarget.value);
    },
    [],
  );

  // this allows handlePress to never change referential equality
  const payload = useRef({ id: lastId + 1, title: title });
  payload.current = { id: lastId + 1, title: title };

  const handlePress = useCallback(
    async (e: React.KeyboardEvent<HTMLInputElement>) => {
      if (e.key === 'Enter') {
        ctrl.fetch(TodoResource.create, payload.current).then(() => {
          console.log(
            'response',
            ctrl.getResponse(
              TodoResource.create,
              payload.current,
              ctrl.getState(),
            ),
            ctrl.getState(),
          );
        });
        setTitle('');
      }
    },
    [ctrl],
  );

  return (
    <TodoBox>
      <input type="checkbox" name="new" checked={false} disabled />{' '}
      <TitleInput
        type="text"
        value={title}
        onChange={handleChange}
        onKeyPress={handlePress}
      />
    </TodoBox>
  );
}
const TodoBox = styled.div`
  text-align: left;
  display: flex;
`;
const TitleInput = styled.input`
  flex: 1 1 auto;
  width: 100%;
  background: #e1e1e1;
  &:focus {
    background: eeeeee;
  }
`;

No need to troubleshoot unless it's helpful for you, I am going to revert to explicitly typing the return type from controller.fetch at the call site for now!

from data-client.

Typing

RestEndpiont typing arguments lets you control the body as well using 'body' argument. You can even set 'searchparameters' to add to the types:

interface TodoInterface {
  title: string;
  completed: boolean;
}
const createTodo = new RestEndpoint({
  path: '/',
  method: 'POST',
  searchParams: { } as { shouldExplode: boolean },
  body: {} as TodoInterface,
  process(value): TodoInterface {
    return value;
  },
});

// POST /?shouldExplode=false
const todo = await createTodo({shouldExplode: false}, {title: 'hi', completed: false});
todo.title;

ctrl.getState()

ctrl.getState() only returns state for already committed updates. This is sort of why we haven't introduced an API that uses this yet, because often you want to do things in the same batch update as the fetch itself, which is what it currently allows you to do. The plan is to have another promise you can await for the updates, but it's unclear the best API for this. Open to suggestions of course. This is really a design problem rather than any technical limitation. Another thing we could do is simply run normalize/denormalize before the state update. However, this won't just be wasted computation, as any interaction with the existing state will update the result....which means if you don't wait for the actual state update, there are numerous race conditions that could make your return value not match what's actually in the store. Obviously many use cases of this won't really care about these problems....which is why I've been asking almost everyone about their exact use cases so we can get something that is a more explicit workaround. If we simply return the value without having that store integrity it would get quite confusing and be nearly impossible to debug.

As a workaround you can use setTimeout:

  const handlePress = useCallback(
    async (e: React.KeyboardEvent<HTMLInputElement>) => {
      if (e.key === 'Enter') {
        ctrl.fetch(TodoResource.create, payload.current).then(() => {
          setTimeout(() => {
            console.log(
              'response',
              ctrl.getResponse(
                TodoResource.create,
                payload.current,
                ctrl.getState(),
              ),
              ctrl.getState(),
            );
          },50)

        });
        setTitle('');
      }
    },
    [ctrl],
  );

Updating the docs entry since this was misleading.

Idea

What I think would likely be best is maybe something like (so there's a clear two step process)

await ctrl.fetch(TodoResource.create, payload.current);
// do something before commit
const { data: storeValue } = await ctrl.awaitResponse(TodoResource.create, payload.current);
// do something after commit

Would love your thoughts on this.

from data-client.

Thank you! Using a combination of body and process in my endpoint config I was able to get the typings to work out how I hoped. It might be good to update the Github example to use this strategy. Maybe I'll do so if I get a chance :)

One thing I'll add is that for endpoints without url/search params, I used searchParams: undefined as any which seemed to provide the proper typings for fetch.

The API in your idea looks pretty good! If it weren't for the setTImeout I would probably be using that in this case, but if awaitResponse or similar is implemented I'll probably use that instead of fetch when I need to use the denormalized value inline with the fetch call.

Thank you for your help and thoughtful responses!

from data-client.

It might be good to update the Github example to use this strategy.

Do you mean also specifying process, because it does already use the 'body' part: https://github.com/coinbase/rest-hooks/blob/master/examples/github-app/src/resources/Comment.ts#L39 Or is there another definition that doesn't use body? I am happy to review PRs tho :)

One thing I'll add is that for endpoints without url/search params, I used searchParams: undefined as any which seemed to provide the proper typings for fetch.

Did omitting searchParams not work correctly?

from data-client.

Oops, you're absolutely right. I think I was confused when originally setting up my base resource and ended up following the pattern of this as any type cast on my resources, rather than taking the approach used by the resources in the Github example.

Omitting searchParams in an endpoint results in the endpoint being typed to expect a search params argument, and it accepts any Partial<MyResource>. In my case, with a create REST endpoint with no url params or search params this is not necessary.

Omitting searchParams:
2 arguments

3 arguments ({ status: 'UNASSIGNED' } is an example of invalid/extraneous params for this REST endpoint and is not part of my API)

Using searchParams: undefined as any (no type error and functions as expected):

from data-client.

Can you show the DeviceResource.create definition? I need that to create a reproduction test case. Thanks!

(the typescript hover things are too long so they cut off to be that useful)

from data-client.

Sure! It looks like this. I'm not going to include all the other values that are referred to here to avoid a massive snippet but let me know if you think one of them is essential for the reproduction.

const create = baseResource.create.extend({
  update: (newId, params) => {
    return {
      [getList.key({ userId: params.userId })]: (
        prevResponse = { items: [] }
      ) => ({
        items: [...prevResponse.items, newId],
      }),
    };
  },
  searchParams: undefined as any,
  body: {} as CreateDeviceBody,
  schema: Device,
  sideEffect: true,
  process(...args: any) {
    return baseResource.create.process.apply(
      this,
      args
    ) as CreateDeviceInterface;
  },
});

from data-client.

I really need to know what baseResource's path is...and I can't see it in any of the hovers. It would also be more useful to see the TypeScript error than what the types are.

Works for me:

const UserResource = createResource({
  path: 'http\\://test.com/groups/:group/users/:id',
  schema: User,
  Endpoint: MyEndpoint,
});
interface CreateDeviceBody {
  username: string;
}
interface UserInterface {
  readonly id: number | undefined;
  readonly username: string;
  readonly email: string;
  readonly isAdmin: boolean;
}

const createUser = UserResource.create.extend({
  update: (newId, params) => {
    return {
      [UserResource.getList.key({ group: params.group })]: (
        prevResponse = { items: [] },
      ) => ({
        items: [...prevResponse.items, newId],
      }),
    };
  },
  //searchParams: undefined as any,
  body: {} as CreateDeviceBody,
  schema: User,
  sideEffect: true,
  process(...args: any) {
    return UserResource.create.process.apply(this, args) as UserInterface;
  },
});
const ctrl = new Controller();
() => ctrl.fetch(createUser, { group: 'hi' }, { username: 'bob' });
() => createUser({ group: 'hi' }, { username: 'bob' });

Since getList and create by default both share the same url path, it's unclear to me (without the createResource definition) how you can have getList take userId, but you want create to not take such an argument. And also without the TS error, it's not clear if that's the problem or something else.

from data-client.

@tobobo Hey, just in case you missed. Without the baseResource I was unable to reproduce the issue. I really just need any of the arguments relating to urls (from https://resthooks.io/rest/api/RestEndpoint#typing - path, schema, process, method, body, searchParams)

from data-client.

@tobobo No pressure, just wondering if this is still an issue, if so is there a detail I missed? Because I cannot reproduce

from data-client.

Hi @ntucker, sorry for the radio silence on this and thank you for your continued help! Looks like I need to reconfigure my GitHub notifications so that I can see this stuff a little better.

Thank you for starting your user example. I'm able to reproduce it by modifying the URL to remove the group param and removing the corresponding { group: 'hi' } argument where applicable. The ctrl.fetch and createUser calls both expect a third argument, even though just including the body should be sufficient (unless there's something I'm missing!)

import { Controller } from '@rest-hooks/react';
import {
  createResource,
  Entity,
  RestEndpoint,
  RestGenerics,
} from '@rest-hooks/rest';

interface UserInterface {
  readonly id: string;
  readonly username: string;
}

class User extends Entity implements UserInterface {
  readonly id: string = '';
  readonly username: string = '';

  pk(): User['id'] {
    return `${this.id}`;
  }
}

const UserResource = createResource({
  path: 'http\\://test.com/users/:id',
  schema: User,
  Endpoint: RestEndpoint,
});

interface CreateUserBody {
  username: string;
}

const createUser = UserResource.create.extend({
  // searchParams: undefined as any,
  body: {} as CreateUserBody,
  schema: User,
  sideEffect: true,
  process(...args: any) {
    return UserResource.create.process.apply(this, args) as UserInterface;
  },
});

const ctrl = new Controller();
const myFetch = () => ctrl.fetch(createUser, { username: 'bob' });
() => createUser({ username: 'bob' });

from data-client.

Can you paste your tsconfig? I just recently fixed some bugs if strictNullChecks: false. Also to clarify, the above code you just pasted is that is broken? Can you paste a screen shot of the error as well?

PS) If you can get it running on some sort of playground, etc, that would reduce the back-and-forth.

from data-client.

@tobobo Released fix in 6.3.3

The critical part of this case was that path had no members

from data-client.

Looking good in 6.3.3. Thank you!

from data-client.