reactive / data-client

Async State Management without the Management. REST, GraphQL, SSE, Websockets
https://dataclient.io
Apache License 2.0
1.93k stars 93 forks source link

Entity.denormalize() is not yet written, so denormalizing a nested Resource is not possible #277

Closed chrisapplegate closed 4 years ago

chrisapplegate commented 4 years ago

React version (e.g., 16.8.5)

16.12

Concurrent mode yes/no

No

Describe the bug

Trying to take advantage of the nesting & normalization features detailed here, however it seems that while data is normalized correctly, it is not denormalized.

To Reproduce

if I have two resources set up as e.g. a PostResource and an AuthorResource:

export class AuthorResource extends Resource {
  id = null;
  name;
  pk() {
    return this.id?.toString();
  }
  static urlRoot = "/api/author";
}

export class PostResource extends Resource {
  id;
  author;
  title;
  pk() {
    return this.id?.toString();
  }
  static urlRoot = "/api/post";
  static schema = {
    author: AuthorResource.asSchema()
  }
}

And I then fetch PostResource using: const posts = useResource(PostResource.listShape(), {}), I note that in the Redux store I am using, the PostResource and AuthorResource objects are correctly normalized and stored under different stoage paths. But when retrieving the PostResource, the AuthorResource is not correctly denormalized and put back into the object. I will just get something like e.g.:

console.log(posts[0]);
>> {
  id: 1,
  author: "1000"
  ...
}

I thought I was doing something wrong until I looked at the source code and found in Entity.ts a TODO: Add denormalizing capability comment on the denormalize static function.

Expected behavior

Expect the returned object to be something like:

{
  id: 1,
  author: {
    id: 1000,
    name: "John Doe"
  }
  ...
}

Additional context

Should I just not bother using normalizing or specifying a schema for nested objects until this is TODO fixed? If so can we get an update to the documentation saying denormalizing is not currently supported?

chrisapplegate commented 4 years ago

Incidentally, I wrote a hacky shim to override denormalize() that seems to work:

class DenormalizableResource extends Resource {
  static denormalize(entity, unvisit) {
    Object.keys(this.schema).forEach((key) => {
      if (key in entity) {
        entity[key] = unvisit(entity[key], this.schema[key])[0];
      }
    });
    return [entity, true];
  }
}

I suspect it might have effects with infinite loops if the child Entity in turn contains a reference to the parent, but it works for my schema types

ntucker commented 4 years ago

This should work:

  static denormalize<T extends typeof Entity>(
    this: T,
    entity: AbstractInstanceType<T> | undefined,
    unvisit: schemas.UnvisitFunction,
  ): [AbstractInstanceType<T>, true] {

    let found = true;
    Object.keys(this.schema).forEach(key => {
      const schema = this.schema[key];
      const [value, foundItem] = unvisit(entity[key], schema);
      if (!foundItem) {
        found = false;
      }
      if (Object.hasOwnProperty.call(entity, key)) {
        entity[key] = value;
      }
    });
    return [entity, found];
  }

This will be incorporated soon, but there's some typing finesse I need to work out, which is why I haven't landed it yet. I don't want to get it wrong and then have to issue an extra breaking release :p

Basically yours is mostly there, but missing the 'found' logic, which is important for results inferring.

chrisapplegate commented 4 years ago

Thanks! I dropped it in, tweaked the types a little bit and got this to work:

  static denormalize<T extends typeof Entity>(
    this: T,
    entity: AbstractInstanceType<T>,
    unvisit: schemas.UnvisitFunction,
  ): [AbstractInstanceType<T>, true] {
    let found = true;
    Object.keys(this.schema).forEach(key => {
      const schema = this.schema[key];
      const [value, foundItem] = unvisit(entity[key as keyof T["prototype"]], schema);
      if (!foundItem) {
        found = false;
      }
      if (Object.hasOwnProperty.call(entity, key)) {
        entity[key as keyof T["prototype"]] = value;
      }
    });
    return [entity, found];
  }
ntucker commented 4 years ago

Related: https://github.com/coinbase/rest-hooks/issues/165

ntucker commented 4 years ago

328