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

Denormalization #7

Closed skoging closed 5 years ago

skoging commented 5 years ago

From what I can see denormalization of resources has been disabled here.

Based on the commit history it appears this was done to fix edge cases. When experimenting with this myself, attempting to denormalize resources fails due to the __ownerID property making it appear as an immutablejs object. Copying the entity instance and deleting the __ownerID property makes denormalization work again.

Making normalizr think the object is an immutablejs object to be intended, but I'm not entirely sure why.

What I'd essentially like is some insight into what the long term plan might be when it comes to denormalization.

I can see the value in exposing normalized entities, perhaps even by default, but right now my issue is that I can't really find a decent way to get the denormalized structure at all.

skoging commented 5 years ago

This is my current denormalize workaround:

const actualDenormalize = (e as any).denormalize;
(e as any).denormalize = function denormalize(entity: any, unvisit: any) {
    // clone the entity and delete the "__ownerID" prop before normalizing
    const clonedEntity = Object.assign({}, entity);
    Object.setPrototypeOf(clonedEntity, Object.getPrototypeOf(entity));
    delete clonedEntity["__ownerID"];

    const denormalized = actualDenormalize.call(e, clonedEntity, unvisit);

    // redefine "__ownerID" on the denormalized object.
    Object.defineProperty(denormalized, "__ownerID", {
        value: 1337,
        writable: false
    });

    return denormalized;
};
ntucker commented 5 years ago

Thanks for creating this issue. I hope my response brings some clarity around this.

The current behavior of faking immutable (which is the only reason __ownerID exists on Resource) is so normalizr returns the original resource rather than spreading to a plain object. This is important for several reasons:

Now for the issue of de-normalizing entities. Currently what I've been doing is using useCache hook. This lets you pull any related resources using the id that ends up in the Resource itself. You can see an example of this here: https://stackblitz.com/edit/rest-hooks?embed=1&file=pages/IssueDetail/CommentsList.tsx but I've pasted the relevant part below.

export default function CommentsList({ issueUrl }: { issueUrl: string }) {
  const comments = useResource(CommentResource.listRequest(), { issueUrl })

  return (
    <React.Fragment>
      {comments.map(comment => <CommentInline key={comment.pk()} comment={comment} />)}
    </React.Fragment>
  )
}

function CommentInline({ comment }: { comment: CommentResource }) {
  const user = useCache(UserResource.singleRequest(), { login: comment.user });
  return (
    <Card style={{ marginTop: 16 }}>
      <Meta
        avatar={<Avatar src={user && user.avatarUrl} />}
        title={<React.Fragment><a href={user.htmlUrl} target="_blank">{comment.user}</a> commented on {moment(comment.createdAt).format("MMM Do YYYY")}</React.Fragment>}
        description={<Markdown source={comment.body} />}
      />
    </Card>
  )
}

What I've found - at least for my uses - is that nesting related entities in a JSON response is usually just a request optimization, and the data is usually useful for rendering some sub-tree of a component. In which case, simply grabbing that part for that component becomes quite easy and natural.

I would love to hear about your use case to inform how I move forward with this as I am not certain exactly the approach I want to take at this moment.

ntucker commented 5 years ago

BTW: if you simply want to change the behavior to treat Resources as plain objects, you just need to reverse the __ownerID attachment in fromJS(). It's sole purpose is to change the denormalization algorithm, so there will be no other side effects. However, it is important to keep in mind that you'll be losing the three things I mentioned above. I'd love to see if your use case could be worked toward without having to lose those things; but that might be the workaround that works best for you in the meantime.

skoging commented 5 years ago

Thanks for the clarity on this. I'm currently evaluating this to replace an existing solution we wrote ourselves, and in the small project I'm testing it on it is working perfectly! The denormalization part was the only thing I found somewhat unexpected, but I see the reasoning behind it.

My solution was essentially the same as you suggest here, by using useCache to extract the necessary resources.

The main issue is that our current solution has no support for denormalization at all, and I was hoping we could gradually migrate towards a normalized consumption of resources. This is not really a big showstopper for us, so we will probably move forward with this anyway.

The only use case I can see currently causing problems is forms for nested datastructures, but I guess in those cases it makes more sense to not normalize the data at all.

skoging commented 5 years ago

I avoided overriding it in fromJs as I didn't know if the immutablejs behaviour was important for normalization as well. But right now this will not work in development due to this check in the selector.

I will stick with avoiding the need for denormalization instead, and if I hit any major issues I'll report back with the use case.

It would probably be useful if this behaviour was more explicitly documented. So that others don't go down the same rabbit hole I did 👍

ntucker commented 5 years ago

I'm thinking of adding this info to this page: https://github.com/coinbase/rest-hooks/blob/master/docs/guides/nested-response.md

Did you find that page yourself and do you think it's clear that this is where that information lies?

ntucker commented 5 years ago

Closing for now; added docs in https://github.com/coinbase/rest-hooks/commit/f65fae45f15ee73e7e97b1101dc7d5afcce5f850

ntucker commented 5 years ago

Also, because of the RequestShape concept, state should technically not rely on anyone using Resources, so I got rid of that check in https://github.com/coinbase/rest-hooks/commit/ab90bb3546614c02b4b16645a3b45b33d7c28964.

PS) And I will eventually look into doing full denormalization.

skoging commented 5 years ago

Thanks for adding this to the docs.

Perhaps finding a way to get the normalized resource without relying on __ownerID would make it easier to implement denormalization at some point in the future.