Skip to content
This repository has been archived by the owner on Nov 17, 2017. It is now read-only.

Support memoization #10

Open
gkaemmer opened this issue Jun 8, 2016 · 11 comments
Open

Support memoization #10

gkaemmer opened this issue Jun 8, 2016 · 11 comments

Comments

@gkaemmer
Copy link

gkaemmer commented Jun 8, 2016

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
Copy link
Owner

gpbl commented Jun 8, 2016

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

@gkaemmer
Copy link
Author

gkaemmer commented Jun 8, 2016

I can maybe take a look this weekend.

@diegohaz
Copy link

diegohaz commented Jun 19, 2016

Currently I'm combining denormalizr with reselect to achieve this

@yachaka
Copy link

yachaka commented Sep 22, 2016

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
Copy link

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
Copy link

yachaka commented Sep 23, 2016

@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
Copy link
Owner

gpbl commented Sep 25, 2016

@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
Copy link

diegohaz commented Sep 25, 2016

+1 the fourth argument is very elegant

@gpbl
Copy link
Owner

gpbl commented Oct 4, 2016

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
Copy link

yachaka commented Oct 4, 2016

@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 gpbl changed the title denormalize should be memoized, changing only when object has actually changed Support memoization Oct 9, 2016
@gpbl
Copy link
Owner

gpbl commented Oct 9, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants