useFetcher getter class inheritance issues

[Jsarihan]

React version
16.13.1

Concurrent mode
No

Rest Hooks version
4.5.5

Describe the bug
Getter inheritance not working as expected with useFetcher. e.g. given a class
ResourceA and a subclass ResourceB extends ResourceA. The object returned by useFetcher(ResourceB.someShape()) is missing getters from ResourceA. This is not an issue with useResource.

To Reproduce
Steps to reproduce the behavior:

1. Create a class

class ResourceA extends SimpleResource {
  readonly active!: boolean;
  ...
  get can_delete() {
    return !this.active;
 }
}

2. Create a subclass

class ResourceB extends ResourceA {
 ...
 // Other properties
}

3. Call useFetcher on resourceB (either promise or async/await, neither work)

const getPosts = useFetcher(ResourceB.listShape());
...
const posts = await getPosts({id: USER_ID});
const deletablePosts = posts.filter((post) => post.can_delete);

Deletable posts is empty regardless of the value of this.active. The behavior works as expected when doing

const posts = useResource(ResourceB.listShape(), {id: USER_ID});
const deletablePosts = posts.filter((post) => post.can_delete);

Expected behavior
I would expect class inheritance of getters to be consistent across useResource and useFetcher. Ideally, subclasses in both cases would have access to the parent getter.

Additional context
Manually setting the object prototype via setPrototypeOf does not work either. Intellisense/TS has no complaints about calling the getters.

ES6 context:
https://stackoverflow.com/questions/53584705/javascript-extending-es6-class-setter-will-inheriting-getter

TS context:
microsoft/TypeScript#338
microsoft/TypeScript#25927

Thanks!

[ntucker]

I believe this may actually be returning a POJO than a class. Can you verify if it can even use getters in the descendant class from useFetcher()?

Changing this to return the class is planned for v5, which will also guarantee referential equality.

[Jsarihan]

You're right. Looks like a POJO, all getters are undefined. Thanks for the context

Is this included in the current v5 beta version?

[ntucker]

Is this included in the current v5 beta version?

Not yet. As a workaround, you could do the following for now (assumes running v5 beta):

import { normalize, denormalize } from '@rest-hooks/normalizr';
import { useFetcher } from 'rest-hooks';

function useBetterFetcher<
  Shape extends FetchShape<Schema, Readonly<object>, any>
>(
  endpoint: Shape,
  throttle = false,
) {
  const fetch = useFetcher(endpoint, throttle);
  return useCallback(async (...args: Parameters<fetch>) => {
    const res = await fetch(...args);
    const { entities, result } = normalize(res);
    return denormalize(result, endpoint.schema, entities);
  }, [ fetch ]);
}

[Jsarihan]

Will give that a shot, thank you!

[ntucker]

Open question

Say a fetch returns an entity that is already in store. In some cases (due to race conditions) you might want to (use the existing entity, because it's actually newer](https://resthooks.io/docs/next/api/SimpleRecord#static-merget-extends-typeof-simplerecordfirst-instancetypet-second-instancetypet--instancetypet) (which can be determined by a timestamp). This already affected what is returned by useResource() or useCache(). However, should it also affect the return value of a fetch?

That is to say, should we

1. Ensure we have the latest state of the consistent cache at the time of resolution
2. Reflect the actual return value from the given fetch

Given that imperative fetches can have side effects, it's not clear which would be better. Should we maybe then provide both? or maybe a different hook for each case?

[yunyu]

@ntucker I'm running into a variant of this issue as well, and my vote is for 2). My rationale is:

• For side-effect free useFetcher calls, the return value is almost always ignored. useFetcher is only used to force trigger an update, and useResource is used for display logic (which is always updated with the latest version from the patch).
• For effectful useFetcher calls, there may be additional data from the call (if you consider an exception as a viable return value) that are not available in the raw useResource. It makes sense to return the denormalized version of the return value in this situation.

[ntucker]

@yunyu I'm a bit confused, because you talk about denormalized version (which is what useResource() returns - includes class properties, etc). but then say #2 - which returns the raw fetch response (which won't have classes). Both can have any additional data from the fetch - just one is processed through the cache and the other is the input to that processing.

[yunyu]

@ntucker Sorry, I realized that I was advocating for a 3rd solution (return a denormalized version of the result from the fetch). What I meant to say is that IMO, the return value of useFetcher should represent the entity at the time when the fetch call succeeds and not subsequent updates.

[yunyu]

There's another context in which this behavior poses a type safety issue. If you fetch a resource with a defined schema field (let's say nestedFoo), the return value of fetch will have a schema field of string when it is actually an object. I think that denormalizing by default makes the most sense here (aka the useBetterFetcher behavior above)

[ntucker]

Tracking solution in #760

[ntucker]

Resolution from useFetch and ctrl.fetch() now resolves to a denormalized form (tho does not incorporate any existing entities from state).

This was first introduced in rest hooks 8. Though not all schemas work perfectly until data client 0.11