Added recursive dependencies support for denormalize method #220
Conversation
There was a problem hiding this comment.
Thanks for looking into this! Unfortunately, it doesn't work for me, as-is. I'd like to see another pass that removes the reserved __cache key from entities and not pushing an empty object in place of something that would be recursive. I'd expect to see the ID left in place.
| "reports": Array [ | ||
| Object { | ||
| "title": "Weekly report", | ||
| "user": Object {}, |
There was a problem hiding this comment.
I would expect this to be the user's ID, not an empty object. I don't think this is actually working the way you want.
| } | ||
|
|
||
| if (!entities.__cache[skey][object[key]]) { | ||
| entities.__cache[skey][object[key]] = {}; |
There was a problem hiding this comment.
Line is unnecessary since you're definiting a value for it on the next line.
There was a problem hiding this comment.
Actually this line block the recursion, but I agree with you that {} is unexpected value. Fixed this by passing object from entities to it.
| if (Array.isArray(object[key])) { | ||
| object[key] = unvisit(object[key], localSchema, entities); | ||
| } else { | ||
| const skey = localSchema.key; |
There was a problem hiding this comment.
What does skey stand for? No need to shorten variable names and sacrifice clarity.
| @@ -72,5 +72,6 @@ export const denormalize = (input, schema, entities) => { | |||
| return input; | |||
| } | |||
|
|
|||
| entities = { ...entities, __cache: {} }; | |||
There was a problem hiding this comment.
I don't like adding a reserved key here. There's no reason someone might have a __cache entity. (I know, it's not the best name for an entity, but I'd hate to ruin someone's day if they tried to use it).
I'd rather see a separate visitedEntities cache object passed around than using reserved keys.
| title: 'Weekly report', | ||
| user: 1 | ||
| }, | ||
| 2: { |
There was a problem hiding this comment.
Why do you have all of this extra setup data (report 2, user 2). Can delete since it's not being used.
| @@ -33,6 +33,26 @@ Object { | |||
| } | |||
| `; | |||
|
|
|||
| exports[`denormalize denormalizes recursive dependencies 1`] = ` | |||
| Object { | |||
There was a problem hiding this comment.
My expected output would be:
{
title: "Weekly report",
user: {
role: 'manager',
reports: [ 1 ]
}
}This is going too many levels deep and isn't properly keeping track of the fact that we've already come across report 1.
There was a problem hiding this comment.
It's reasonable. I changed denormalization logic to confirm described expectations. Please, review last commit.
There was a problem hiding this comment.
Thanks for getting this together, and great work, @maxsbelt!
I'm going to merge this and do a tiny bit of refactoring to make this work a little more like the reverse of addEntities from the normalization process so that the logic for tracking what entities has been visited is centralized outside of the schemas.
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Problem
denormalizemethod doesn't support recursive linked entities.Solution
__cachekey is added toentitiesobject parameter for keeping already visited entities. The same approach is already used in denormalizr package.