Adding memoization #20

yachaka · 2016-09-28T16:06:08Z

Hello,

This is the PR discussed in issue #10.
API follows what has been discussed : { memoized: true } to activate memoization.

I've been rather busy so I just added few tests and the overall code could be refactored in some points somehow but it does the job.

Needs the docs to be partly rewritten though to explain how to use/the advantages of using memoization.

Note: the cache is for now global. In the future. It would be necessary to take is as argument to allow the user to maintain a cache for each set of entity he might be using.

gpbl · 2016-09-28T18:19:06Z

Awesome @yachaka, thanks a lot! Please give some some time to review the changes.

gpbl · 2016-10-04T11:54:57Z

@yachaka Thanks again for your PR. Yes, I see the need of a refactoring to avoid some code dupes... If you are have some time for a code review, we can work on this together, otherwise I can try updating the branch myself in the next days.

clessg · 2016-10-04T13:52:51Z

@yachaka Thanks for this. Really looking forward to this change. 🎉

yachaka · 2016-10-04T14:44:54Z

@clessg Thanks for the feedback ! Really appreciated ! :)
@gpbl I can look onto refactoring a little next week, if no one does !

gpbl · 2016-10-09T21:25:38Z

Sorry, I've broken the PR fixing up some code in master :( The valuable code is still visible and reusable from the diff.

gpbl

Added some ideas for refactoring 😄

gpbl · 2016-10-09T21:26:25Z

.babelrc

@@ -1,3 +1,3 @@
 {
-  "presets": ["es2015"]
+  "presets": ["es2015", "stage-0"]


I don't think we need stage-0 💣 :)

Sure ! I don't remember why I added it, I used ...spread at some point I think.

gpbl · 2016-10-09T21:27:55Z

package.json

@@ -6,6 +6,7 @@
  "scripts": {
    "prebuild": "rimraf dist lib",
    "build": "babel src --out-dir lib",
+    "mocha": "mocha --compilers js:babel-core/register",
    "test": "mocha --compilers js:babel-core/register --recursive",


thanks! indeed, no need to --recursive here!

gpbl · 2016-10-09T21:28:55Z

src/index.js

@@ -1,12 +1,14 @@
 import IterableSchema from 'normalizr/lib/IterableSchema';
 import EntitySchema from 'normalizr/lib/EntitySchema';
 import UnionSchema from 'normalizr/lib/UnionSchema';
+import assign from 'lodash/assign';


Does babel.js transpile Object.assign()? 🤔
Because I'd better use it than adding another dependency.

No, you need a polyfill ! But that's probably better to include one than using a library function.

gpbl · 2016-10-09T21:33:44Z

src/index.js

@@ -21,13 +23,15 @@ function resolveEntityOrId(entityOrId, entities, schema) {

  if (isObject(entityOrId)) {
    id = getIn(entity, [schema.getIdAttribute()])
+    entity = getIn(entities, [key, id])
  } else {
    entity = getIn(entities, [key, id])
  }



Alright! Then I'd better write

if (isObject(entityOrId)) { id = getIn(entity, [schema.getIdAttribute()]); } entity = getIn(entities, [key, id]);

gpbl · 2016-10-09T21:42:36Z

src/index.js

-  const itemSchema = schema.getItemSchema();
+function denormalizeUnion(entity, entities, unionSchema, bag) {
+  if (!entity.schema)
+    throw new Error('Expect `entity` to have a schema key as a result from normalizing an union.')


Is this change related to the memoization? 🐺

I don't think so, I think I just wanted to simplify the function.

gpbl · 2016-10-09T21:46:39Z

src/index.js

@@ -50,14 +73,40 @@ function denormalizeIterable(items, entities, schema, bag) {
 * @param   {object} bag
 * @returns {object|Immutable.Map}
 */
-function denormalizeUnion(entity, entities, schema, bag) {
-  const itemSchema = schema.getItemSchema();
+function denormalizeUnion(entity, entities, unionSchema, bag) {


We should add an options parameter here:

denormalizeUnion(entity, entities, unionSchema, options bag)

and pass it some lines below to denormalize(...

gpbl · 2016-10-09T21:54:25Z

src/index.js

+    if (cache[key][id].entity !== entity) {
+      cache[key][id].entity = entity
+      cache[key][id].denormalized = entity
+    }


Caching should be abstracted in another file (e.g. cache.js) to simplify this part and to not expose the caching mechanism here. Something like cache.get(), cache.set(), cache.clean().

Sure ! Feel free to export this part of the code.

gpbl · 2016-10-09T21:54:39Z

src/index.js

+    let referenceObject = cache[key][id].denormalized
+    let relationsToUpdate = {}
+
+    /* For each relation in EntitySchema */


I'm not sure what do you mean with relation. Is that an entity?

Anyway, here I'm lost 😄 I don't fully understand the reasoning behind the code below...

relation is a relation in the entity Schema, which are Entities, yes 👍 . I fill this object with all the relations that have been updated - in the loop below - because we want to return the original object if nothing changed !

gpbl · 2016-10-09T22:00:54Z

src/index.js

+/**
+ * Same as denormalize right above, but memoized
+ */
+function denormalizeMemoized(obj, entities, schema, bag = {}) {


If we add an options argument here, we could skip the various denormalize*Memoized() functions and merge their code into the existing denormalize*():

export function denormalize(obj, entities, schema, options, bag = {}) { if (obj === null || typeof obj === 'undefined' || !isObject(schema)) { return obj; } if (schema instanceof EntitySchema) { return denormalizeEntity(obj, entities, schema, options, bag); } else if (schema instanceof IterableSchema) { return denormalizeIterable(obj, entities, schema, options, bag); } else if (schema instanceof UnionSchema) { return denormalizeUnion(obj, entities, schema, options, bag); } // Ensure we don't mutate it non-immutable objects const entity = isImmutable(obj) ? obj : merge({}, obj); return denormalizeObject(entity, entities, schema, options, bag); }

gpbl · 2016-10-09T22:09:48Z

src/index.js

  return denormalize(
-    Object.assign({}, entity, { [entity.schema]: entity.id }),
+    trueEntity,


Why do we need the trueEntity? I think a code comment here would be helpful.

yachaka · 2016-10-10T20:09:06Z

I will be able to look into refactoring along your comments this friday. ;)

diegohaz · 2016-11-29T12:46:06Z

Hey guys. Any updates on this?

yachaka · 2016-11-29T12:49:46Z

@diegohaz I've been a lot busy lately (just started a new job), but I will into refactoring this week (promess 😸). For now, you can use my fork (yachaka/denormalizr) ; it works with memoization using { memoized: true } as the first argument.

coveralls · 2016-12-10T15:11:05Z

Coverage decreased (-8.4%) to 91.603% when pulling 8238d50 on yachaka:master into 08ede86 on gpbl:master.

coveralls · 2016-12-10T15:16:32Z

Coverage decreased (-8.4%) to 91.603% when pulling ee2d493 on yachaka:master into 08ede86 on gpbl:master.

yachaka · 2016-12-10T15:19:01Z

@gpbl Updated README, added some code comment. I didn't refactor the denormalize entry point function ; I think it is more readable this way - splitted into 2 functions. In term of performance, it also eliminates a check. Please review 👍

…st-spread

coveralls · 2016-12-10T15:23:47Z

Coverage decreased (-8.4%) to 91.603% when pulling 95b7283 on yachaka:master into 08ede86 on gpbl:master.

yachaka · 2017-01-26T12:36:34Z

@gpbl Sup ?

yachaka added 2 commits September 28, 2016 18:01

Added memoization

8a01be1

Removed old temporary file

f610958

gpbl mentioned this pull request Oct 4, 2016

Support memoization #10

Open

gpbl reviewed Oct 9, 2016

View reviewed changes

Up-to-date with upstream. Updated Readme, added code comment.

8238d50

Readme adjusts

ee2d493

Removed stage-0 in favor for more specific plugin transform-object-re…

95b7283

…st-spread

diegohaz mentioned this pull request Dec 12, 2016

Add memoization for entities tramaLabs/web#31

Open

dxinteractive mentioned this pull request Dec 19, 2016

Memoization 92green/enty#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding memoization #20

Adding memoization #20

yachaka commented Sep 28, 2016 •

edited

Loading

gpbl commented Sep 28, 2016

gpbl commented Oct 4, 2016

clessg commented Oct 4, 2016

yachaka commented Oct 4, 2016

gpbl commented Oct 9, 2016

gpbl left a comment

gpbl Oct 9, 2016 •

edited

Loading

yachaka Oct 10, 2016

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

yachaka Oct 10, 2016

gpbl Oct 9, 2016

gpbl Oct 9, 2016 •

edited

Loading

yachaka Oct 10, 2016

gpbl Oct 9, 2016

gpbl Oct 9, 2016 •

edited

Loading

yachaka Oct 10, 2016

gpbl Oct 9, 2016 •

edited

Loading

yachaka Oct 10, 2016

gpbl Oct 9, 2016 •

edited

Loading

yachaka Oct 10, 2016

gpbl Oct 9, 2016

yachaka commented Oct 10, 2016

diegohaz commented Nov 29, 2016

yachaka commented Nov 29, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

yachaka commented Dec 10, 2016

coveralls commented Dec 10, 2016

yachaka commented Jan 26, 2017

Adding memoization #20

Are you sure you want to change the base?

Adding memoization #20

Conversation

yachaka commented Sep 28, 2016 • edited Loading

gpbl commented Sep 28, 2016

gpbl commented Oct 4, 2016

clessg commented Oct 4, 2016

yachaka commented Oct 4, 2016

gpbl commented Oct 9, 2016

gpbl left a comment

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpbl Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yachaka commented Oct 10, 2016

diegohaz commented Nov 29, 2016

yachaka commented Nov 29, 2016

coveralls commented Dec 10, 2016

coveralls commented Dec 10, 2016

yachaka commented Dec 10, 2016

coveralls commented Dec 10, 2016

yachaka commented Jan 26, 2017

yachaka commented Sep 28, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading

gpbl Oct 9, 2016 •

edited

Loading