denormalizr icon indicating copy to clipboard operation
denormalizr copied to clipboard

Support memoization

Open gkaemmer opened this issue 7 years ago • 11 comments

In my mind, denormalizr is most useful when passing large nested objects to a component using connect. But currently it always returns a new object, so the component re-renders on every action.

denormalize should always return the same (strictly equal) result unless:

  1. The root object is different
  2. Any of the associations in the schema are different

I believe this could be accomplished fairly simply using a cache of denormalized resources.

gkaemmer avatar Jun 08 '16 16:06 gkaemmer

Yes, this is needed 👍 I'm not a champion in caching techniques, do you have a suggestion about how to implement it?

gpbl avatar Jun 08 '16 17:06 gpbl

I can maybe take a look this weekend.

gkaemmer avatar Jun 08 '16 18:06 gkaemmer

Currently I'm combining denormalizr with reselect to achieve this

diegohaz avatar Jun 19 '16 00:06 diegohaz

Hello,

I've currently forked and achieved memoization with denormalizr. I will push it soon on my repo, but I was unable to make it work with circular dependency ; I don't think it is possible for technical reasons on how the cache diff is calculated.

Is it a concern for you ? Dropping circular dependencies would introduce a breaking change. But memoization is a great plus.

yachaka avatar Sep 22 '16 14:09 yachaka

Can't you use reselect? I didn't test it specifically, but I'm pretty sure that it handles circular dependencies:

import { createSelector } from 'reselect'
import { denormalize } from 'denormalizr'

const getChallenge = createSelector(
  getEntitiesState,
  getChallengeState,
  (entities, challenge) => denormalize(challenge, entities, challengeSchema)
)

diegohaz avatar Sep 22 '16 18:09 diegohaz

@diegohaz With cache, you can achieve 2 things : recompute only when the concerned entities have been changed, and recompute only the part of the denormalized entity which has changed.

reselect will achieve first, but only if you do not only watch the entities state, and combine it with an other specific part of the state (like you did, with getChallengeState) that will not get updated every other action. It can't achieve second, as this is the job of the denormalize function to reuse previous objects.

For why it is important for performance to only recompute the part that has changed, let's imagine a Schema :

const Book = new Schema('books')
const Author = new Schema('authors')
const Review = new Schema('reviews')

Book.define({
  author: Author,
  reviews: arrayOf(Review),
})

With the cache, this will work:

let response = {
  id: 1,
  title: 'Game of Thrones',
  author: {
    id: 1,
    name: 'Georges RR Martin'
  },
  reviews: [{
    id: 1,
    content: 'Great book',
  }]
}

let normalized = normalize(response, Book)

let denormalized1 = denormalize(normalized.result, normalized.entities, Book)
let denormalized2 = denormalize(normalized.result, normalized.entities, Book)

// Every object in the 2 denormalized response are strictly equal
assert(denormalized1 === denormalized2
  && denormalized1.author === denormalized2.author
  && denormalized1.reviews === denormalized2.reviews
 && denormalized1.reviews[0] === denormalized2.reviews[0])

// We change only the review entity with id 1
normalized.entities.reviews[1] = {
   ...normalized.entities.reviews[1],
  content: 'I changed my mind, this sucks !',
}

let denormalized3 = denormalize(normalized.result, normalized.entities, Book)

// The root object and the reviews relation object are new
assert(denormalized2 !== denormalized3
  && denormalized2.reviews !== denormalized3.reviews)
// ... and author object gets unchanged
assert(denormalized2.author === denormalized3.author)

This essentially means that we will only trigger re-render on our React component and all his children that might exploit some part of the nested response only if the denormalized response, or the part, truly changed.

PS: Circular dependency might be doable but would introduce extra traversal.

yachaka avatar Sep 23 '16 11:09 yachaka

@yachaka thanks for caring about this!

I'm ok for disabling circular dependencies if case memoization is enabled. I'm thinking to export another function enabling memoization, which turns circular dependencies off. Something like:

import { memoizedDenormalize } from 'denormalizr';
const denormalized = memoizedDenormalize(normalized.result, normalized.entities, Book);

memoizedDenormalize would be a shortcut for a fourth options argument for denormalize, so the code above would be the same as:

import denormalize from 'denormalizr';
const denormalized = denormalize(normalized.result, normalized.entities, Book, { memoize: true });

gpbl avatar Sep 25 '16 05:09 gpbl

+1 the fourth argument is very elegant

diegohaz avatar Sep 25 '16 07:09 diegohaz

The solution by @yachaka in #20 works, but it needs some refactoring to keep the code more maintainable.

I wonder if it makes sense supporting external cache libraries such as https://github.com/isaacs/node-lru-cache, thus implementing a similar API for an internal cache utility.

I'm marking this issue as "breaking change" because the export would work differently – maybe once memoization is ready we can release a v1.0 🎉

gpbl avatar Oct 04 '16 12:10 gpbl

@gpbl Shouldn't it be considered a new feature rather than a breaking change ? The old API is fully supported and existing code will not break.

yachaka avatar Oct 04 '16 14:10 yachaka

Shouldn't it be considered a new feature rather than a breaking change

@yachaka sure! I was thinking to add a new options argument:

denormalize(entity, entities, schema, { memoize = false })

yet now the fourth argument is a bag (which shouldn't be there, actually). Anyway, we are still in the v0. realms so I wouldn't care so much about this :)

gpbl avatar Oct 09 '16 21:10 gpbl