infinum / datx

DatX is an opinionated JS/TS data store. It features support for simple property definition, references to other models and first-class TypeScript support.
https://datx.dev
MIT License
140 stars 7 forks source link

useMutation first parameter type error #1175

Closed doroz0 closed 1 year ago

doroz0 commented 1 year ago

Used libraries

core, jsonapi, swr

Library version(s)

@datx/core 2.5.1, @datx/jsonapi 2.5.1, @datx/swr 2.5.1

Sample API response (if relevant)

No response

Environments with the issue

All

Environments without the issue

No response

Current behavior

export const createPost = (client: JsonapiSwrClient, body: string) => {
  const model = new Post({ body });
  const url = getModelEndpointUrl(model);
  const data = modelToJsonApi(model);

  return client.request<Post>(url, "POST", { data });
};

const [create, { status: createStatus }] = useMutation(createPost, { // <---------------
  onSuccess: async () => {
    const input = inputRef.current;
    if (input) input.value = "";
    mutate();
  },
});

image

Expected behavior

No errors

Reproduction steps

You can try it out at https://github.com/doroz0/soc-net-clone-nextjs, just remove the `as any` (https://github.com/doroz0/soc-net-clone-nextjs/blob/main/src/pages/index.tsx#L24)
kristian240 commented 1 year ago

The issue with this error is that createPost method is using client.request. The return type of client.request is a Response which has either to one or to many relationship. With @datx/swr we have created 2 new responses - SingleResponse and CollectionResponse. To use them, change the client.request to client.requestSingle or client.requestCollection and the type will be correct.

I tried to override the request method to enhance the DX with this PR but there are other issues that occurred and that are not that easy to solve quickly since a lot of internals are using client.request and we are doing castings on another level.

Next idea is that to create a eslint rule to solve throw an error when developer is using client.request in a project that has @datx/swr installed, what do you think about that?

One other solution is to check if a client.request has been called with fetchOptions: { Response: CollectionResponse/SingleResponse } which will indicate that this is an okay call to make and throw an error if called without - but that does not seem right and this is also causing us to ship more code just to fix the typing issues.

@isBatak @DarkoKukovec @doroz0 what you say on all of this?

DarkoKukovec commented 1 year ago

I didn't notice many places where we're using the request method internally. I changed this behavior and updated a test that failed, and everything seems to be working fine? https://github.com/infinum/datx/commit/94978ed760696fbcceeeda8b850f46fffe37a243

kristian240 commented 1 year ago

Thanks a lot, @DarkoKukovec :D Would this be considered a breaking change if we are throwing errors?

DarkoKukovec commented 1 year ago

Yeah, I guess so... 🤔

1179 is also still pending because it is a breaking change