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

Request parameters in fetch #87

Closed
dennari opened this issue Jun 19, 2019 · 3 comments
Closed

Request parameters in fetch #87

dennari opened this issue Jun 19, 2019 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@dennari
Copy link

dennari commented Jun 19, 2019

Is your feature request related to a problem? Please describe.
I want to include additional parameters which are not included in the server response in the Resource as described in https://resthooks.io/docs/guides/network-transform#case-of-the-missing-id. However parsing them out from the url seems really burdensome and can be impossible, since the request parameters could also be in the body in some cases.

Describe the solution you'd like
As a workaround, I whipped up this static method in my Resource:

  static customRequest<T extends typeof Resource>(
    this: T,
    params: RequestParameters,
  ): ReadShape<SchemaBase<AbstractInstanceType<T>>> {
    const self = this;
    const getUrl = (p: Readonly<object>) => {
      return self.url() + '/' + pk(p);
    };
    const schema: SchemaBase<AbstractInstanceType<T>> = this.getEntitySchema();
    const options = this.getRequestOptions();
    return {
      type: 'read',
      schema,
      options,
      getUrl,
      async fetch(_url: string, _body?: Readonly<object>) {
        const jsonResponse = await self.fetch('post', self.url(), params);
        return {
          ...params,
          ...jsonResponse,
        };
      },
    };
  }

Describe alternatives you've considered
Haven't considered other solutions, but I'm sure there's some cleaner pattern.

Additional context

@dennari dennari added the enhancement New feature or request label Jun 19, 2019
@ntucker
Copy link
Collaborator

ntucker commented Jun 19, 2019

I definitely feel your pain. I've seen many people be annoyed they have to parse out the params they just had for this. I'm considering shifting the interface around a bit so fetch has the actual params.

To address the sample code: It's a clever workaround, but kind of breaks the current assumptions of RequestShape - as something that doesn't change. This won't be a problem in some scenarios, but for instance when using with imperative useFetcher(), it will result in a memoized callback that isn't updated based on the closure around params. (https://github.com/coinbase/rest-hooks/blob/master/src/react-integration/hooks/useFetcher.ts#L58 doesn't include requestShape.fetch). While this is a kind of bad assumption to make, it's sort of the work around for the fact that the current requestshapes always get regenerated even tho they don't change dynamically. More importantly tho - even if this would change in the future, having params in the requestshape itself would be limiting as it wouldn't allow any optimizations on keeping them the same - which for instance is why the fetch() function returned by useFetcher() is what takes in params - so the fetch itself can remain consistent.

I'm curious to hear about the 'impossible' scenario as I haven't run into that yet. If you are willing I would love to hear about your specific use-case where it is not just burdensome and annoying but actually not possible to get the params back. This would help me holistically consider things as I think about tweaks to the design.

Now to address my current thoughts on a solution:

  • I'm thinking of renaming getUrl to something like getSerializedRequest or getRequestKey or getKey. This would strictly be for mapping a request in the cache. This distinction from url is important as we aim to support more protocols like protobuf GRPC or GraphQL as they often use one url, but signify requests in another manner.
  • Next I would change the fetch to be (params, body). When I was originally designing this that seemed to be redundant as REST clients would simply call the same url function inside the fetch. However, even with rest ideally your lookup key would also include the method type (like 'GET /users/') so even there the distinction becomes clear.

I would love to hear your thoughts on my current solution ideas as well as explaining this 'impossible' case you have run into. Thanks for the detailed report!

@ntucker
Copy link
Collaborator

ntucker commented Jul 7, 2019

If you have a spare moment, I'd love to hear your thoughts on #94. I am preparing for a 2.0 release soon and want to make sure this solves your issues :)

@ntucker
Copy link
Collaborator

ntucker commented Jul 9, 2019

Closed in #94

@ntucker ntucker closed this as completed Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants