gpbl / denormalizr

Denormalize data normalized with normalizr
https://www.npmjs.com/package/denormalizr
MIT License
228 stars 24 forks source link

Support memoization #10

Open gkaemmer opened 8 years ago

gkaemmer commented 8 years ago

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.

gpbl commented 8 years ago

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

gkaemmer commented 8 years ago

I can maybe take a look this weekend.

diegohaz commented 8 years ago

Currently I'm combining denormalizr with reselect to achieve this

yachaka commented 7 years ago

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.

diegohaz commented 7 years ago

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)
)
yachaka commented 7 years ago

@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.

gpbl commented 7 years ago

@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 });
diegohaz commented 7 years ago

+1 the fourth argument is very elegant

gpbl commented 7 years ago

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 🎉

yachaka commented 7 years ago

@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.

gpbl commented 7 years ago

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 :)