Support denormalizing from immutable entities #228

jeffcarbs · 2017-01-25T20:56:27Z

Fixes #222

Problem

First I want to say that I've been using and following this repo for quite a while and it's great to see denormalize finally land!

It seems that most places I've seen that suggest using normalizr also suggest using an immutable store for entities. However, in doing so you lose the ability to use denormalize. Adding immutable as a dependency would add complexity and bloat for users who are not using immutable data.

Solution

Update denormalize to handle both mutable and immutable entities without actually requiring immutable as a dependency.

Support is handled by using simple getIn, setIn, hasIn methods that defer to the immutable implementations if the object is immutable otherwise implement the same behavior on plain javascript objects/arrays. immutable-js is required as a dev dependency since it's needed for specs. I just updated the existing specs to also test with immutable entities.

The best part is that it only adds ~70 bytes (compressed) and ~40 bytes (minified + compressed) and doesn't add any external dependencies.

TODO

Add & update tests
Ensure CI is passing (lint, tests, flow)
Update relevant documentation

Size Diff

Before

Package:      dist/normalizr.js
Bundle Size:  18.06 KB
Compressed:   3.93 KB

Package:      dist/normalizr.min.js
Bundle Size:  8.76 KB
Compressed:   2.48 KB

After

Package:      dist/normalizr.js
Bundle Size:  19.31 KB
Compressed:   4.09 KB

Package:      dist/normalizr.min.js
Bundle Size:  9.25 KB
Compressed:   2.61 KB

coveralls · 2017-01-25T20:58:08Z

Coverage remained the same at 100.0% when pulling 15a013a on jcarbo:feature/immutable into e365e51 on paularmstrong:master.

jeffcarbs · 2017-01-25T21:09:13Z

src/schemas/__tests__/__snapshots__/Array.test.js.snap

+  Object {
+    "id": 1,
+    "name": "Milo",
+  },


It seems that immutable data structures are converted to plain objects when serialized into snapshots, see: jestjs/jest#1622. In this case, the Array should be an actual array since the test uses [1,2] as input but each object is actually an Immutable.Map (which I manually verified is the case).

paularmstrong · 2017-01-26T23:26:15Z

Nice work @jcarbo! For the most part this looks good. I have a few nit requests, I think, but I'd like to spend more time with it once I'm back (currently traveling till next week)

paularmstrong

This will need rebase, as denormalize has changed a bit to handle recursive schema definitions.

I'm also really concerned about edge-cases here, since the fallback checks are very much not fool-proof.

Couldn't we set a flag at the top level of the denormalize method that can pass down whether the entities provided are immutable or not? Is it safe to assume when one entity is immutable, that all are immutable as well?

paularmstrong · 2017-01-30T20:20:45Z

src/schemas/Entity.js

-    const entity = typeof entityOrId === 'object' ? entityOrId : entities[this.key][entityOrId];
-    const entityCopy = { ...entity };
+    const entity = typeof entityOrId === 'object' ? entityOrId : getIn(entities, [ this.key, entityOrId ]);
+    let entityCopy = isImmutable(entity) ? entity : { ...entity };


Avoid using let, please

paularmstrong · 2017-01-30T20:21:46Z

src/schemas/Entity.js

    Object.keys(this.schema).forEach((key) => {
-      if (entityCopy.hasOwnProperty(key)) {
+      if (hasIn(entityCopy, [ key ])) {


Can we use the isImmutable(entity) method before doing getIn, hasIn, and setIn to avoid unnecessary overhead?

paularmstrong · 2017-01-30T20:22:22Z

src/schemas/ImmutableUtils.js

+ * @return {bool}
+ */
+export function isImmutable(object) {
+  return !!(object && object.getIn);


edge-case: what if someone has a key, getIn on their entity? (also true for setIn and hasIn.

paularmstrong · 2017-01-30T20:23:18Z

src/schemas/Object.js

@@ -13,14 +15,14 @@ export const normalize = (schema, input, parent, key, visit, addEntity) => {
 };

 export const denormalize = (schema, input, unvisit, entities) => {
-  const object = { ...input };
+  let inputCopy = isImmutable(input) ? input : { ...input };


nit: avoid let statements.

paularmstrong · 2017-01-30T20:23:34Z

src/schemas/Object.js

@@ -1,3 +1,5 @@
+import { getIn, hasIn, isImmutable, setIn } from './ImmutableUtils';


nit: import * as ImmutableUtils from './ImmutableUtils';

coveralls · 2017-02-03T02:58:56Z

Coverage remained the same at 100.0% when pulling 4debf74 on jcarbo:feature/immutable into b4dd248 on paularmstrong:master.

coveralls · 2017-02-03T04:10:30Z

Coverage remained the same at 100.0% when pulling d2da777 on jcarbo:feature/immutable into b4dd248 on paularmstrong:master.

jeffcarbs · 2017-02-03T04:27:15Z

@paularmstrong - thanks for reviewing! I rebased and got the tests back to green.

Also, reading through your comments it seems that your (valid) concerns were around performance/overhead and robustness. The commit I just pushed up takes a slightly different approach and should address your concerns:

to reduce false positives, make the isImmutable check much more specific. This is a little brittle given it's relying on some internals of Immutable.Map and Immutable.Record but I think the tradeoff is more than worth it to avoid bringing in immutable as a dependency. Also the immutable library is one of the more stable ones I've seen in the javascript world so it's unlikely we'll be affected.
There was a lot indirection introduced so that the same denormalize functions could handle both immutable and mutable objects. Instead, at the top of the denormalize function in the Entity and Object schemas we first check if the object is immutable and, if so, pass it to a separate denormalizeImmutable record.

With these changes, the compressed size increased by another ~90 bytes. Still not that bad imho.

coveralls · 2017-02-03T04:30:17Z

Coverage remained the same at 100.0% when pulling 72365c1 on jcarbo:feature/immutable into b4dd248 on paularmstrong:master.

paularmstrong

Looks good. A couple nits marked inline, but I don't think we'll need any major changes. I'll test more when I get some time.

paularmstrong · 2017-02-03T14:01:39Z

src/index.js

+    return entityOrId;
+  }
+
+  if (ImmutableUtils.isImmutable(entities)) {


This could be sent in as a flag, since entities is a wrapped argument in the getEntities function. This would save on this conditional over every entity iteration. Something like...

const getEntities = (entities, visitedEntities) => { const isImmutable = ImmutableUtils.isImmutable(entities); return (schema, entityOrId) => { // ... const entity = getEntity(entityOrId, schemaKey, entities, isImmutable); // ... }; };

paularmstrong · 2017-02-03T14:02:39Z

src/schemas/Entity.js

-    this._getId = typeof idAttribute === 'function' ? idAttribute : (input) => input[idAttribute];
+    this._getId = typeof idAttribute === 'function' ?
+      idAttribute :
+      (input) => ImmutableUtils.isImmutable(input) ? input.get(idAttribute) : input[idAttribute];


Can we avoid nested ternaries, please? They get difficult to read.

paularmstrong · 2017-02-03T14:06:48Z

src/schemas/ImmutableUtils.js

+  return Object.keys(schema).reduce((object, key) => {
+    // Immutable maps cast keys to strings on write so we need to ensure
+    // we're accessing them using string keys.
+    const stringKey = typeof key === 'string' ? key : key.toString();


Would be safe to just force casting it:

const stringKey = `${key}`;

jeffcarbs · 2017-02-03T14:28:57Z

Just pushed those changes 👍

coveralls · 2017-02-03T14:30:34Z

Coverage remained the same at 100.0% when pulling 2d379ab on jcarbo:feature/immutable into b4dd248 on paularmstrong:master.

jeffcarbs · 2017-02-09T15:03:36Z

Friendly nudge on this when you get a chance :)

paularmstrong

Sorry for the slowness and thanks for the nudge! We recently rolled new mobile.twitter.com out to a ton more people!.

Just one little nit request inline. Can you add a case for it? I can get this merged and deployed today if so!

paularmstrong · 2017-02-09T15:30:56Z

src/schemas/ImmutableUtils.js

+export function isImmutable(object) {
+  return !!(object && (
+    object.hasOwnProperty('__ownerID') || // Immutable.Map
+    (object._map && object._map.object.hasOwnProperty('__ownerID')) // Immutable.Record


This condition is never hit by tests. Can you add a case for this?

coveralls · 2017-02-09T16:23:21Z

Coverage remained the same at 100.0% when pulling 99baca4 on jcarbo:feature/immutable into b4dd248 on paularmstrong:master.

jeffcarbs · 2017-02-09T16:29:55Z

Good call on that missing coverage, the conditional was actually broken!

jeffcarbs · 2017-02-09T16:32:52Z

src/index.js

@@ -58,16 +59,30 @@ const unvisit = (input, schema, getDenormalizedEntity) => {
    return method(schema, input, unvisit, getDenormalizedEntity);
  }

+  if (input === undefined || input === null) {


Wanted to call your attention to this change added in the last commit. This came up in the test with immutable records because it always instantiates all fields defined in the schema, using the default value if you didn't give it pass it one. E.g.

> Menu = new Immutable.Record({ id: null, food: null }) [Function: Record] > menuData = { id: 2 } { id: 2 } > nonRecordMenu = menuData { id: 2 } > recordMenu = new Menu(menuData) Record { "id": 2, "food": null }

So while denormalizing the record, it's going to try to unvisit food: null which currently throws an unexpected error in _getId

This assumes that you're not going to have undefined or null resolve to anything during the denormalization process. Is that an ok assumption to make?

Yeah that's a good one and similar to #225. This is good. Thanks!

Another related thing I just realized: this currently throws that same unexpected error if an entity is not found:

const mySchema = new schema.Entity('tacos'); const entities = { tacos: { 1: { id: 1, type: 'foo' } } }; > denormalize(3, mySchema, entities) TypeError: Cannot read property 'id' of undefined at EntitySchema._getId (~/normalizr/dist/normalizr.js:226:65) at EntitySchema.getId (~/normalizr/dist/normalizr.js:245:19) at ~/normalizr/dist/normalizr.js:635:21 at EntitySchema.denormalize (~/normalizr/dist/normalizr.js:273:20) at unvisit (~/normalizr/dist/normalizr.js:612:17) at Object.denormalize$1 [as denormalize] (~/normalizr/dist/normalizr.js:651:10) at repl:1:3 at realRunInThisContextScript (vm.js:22:35) at sigintHandlersWrap (vm.js:98:12) at ContextifyScript.Script.runInThisContext (vm.js:24:12)

I kinda feel like that should just return undefined. In my apps, I think I'd rather just ignore missing data, especially when denormalizing nested data.

I could also see it being configurable so you can handle how you want (e.g. initialize with empty object, return null,undefined, throw error)

denormalize(3, mySchema, entities, { onNotFound: (id, schema) => undefined }) denormalize(3, mySchema, entities, { onNotFound: (id, schema) => { id } }) denormalize(3, mySchema, entities, { onNotFound: (id, schema) => throw new Error(`could not find ${schema.name} with id='${id}'` })

Thoughts? If you agree I'd probably remove this line and just handle that below if an entity is not found in the store.

I think there was a PR for this and it was randomly closed... #229. Makes sense to fix. We don't ever run across that case, but I could see it being possible.

Also, already merged this PR... so maybe a new one?

Sorry, my brain isn't working today... I'd rather stay away from the extra configuration, as you proposed. I'd expect the application to handle receiving null for an entity elsewhere than pushing more into normalizr.

paularmstrong · 2017-02-09T16:42:47Z

Nice job on this! Looks like we've actually increased the performance since the initial release of normalizr2 and this addition doesn't have any ill effects! We used to be right at about 200%. Now looking at 220%.

normalzr2 x 111 ops/sec ±4.77% (70 runs sampled)
normalizr3 x 246 ops/sec ±4.60% (73 runs sampled)

lock · 2018-05-07T15:04:57Z

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.

jeffcarbs commented Jan 25, 2017

View reviewed changes

paularmstrong suggested changes Jan 30, 2017

View reviewed changes

Support denormalizing from immutable entities

4debf74

jeffcarbs force-pushed the feature/immutable branch from 15a013a to 4debf74 Compare February 3, 2017 02:57

Use separate denormalizeImmutable function

72365c1

jeffcarbs force-pushed the feature/immutable branch from d2da777 to 72365c1 Compare February 3, 2017 04:28

paularmstrong reviewed Feb 3, 2017

View reviewed changes

Code review clean up

2d379ab

paularmstrong approved these changes Feb 9, 2017

View reviewed changes

Handle null/undefined, add Immutable.Record specs

99baca4

jeffcarbs commented Feb 9, 2017

View reviewed changes

paularmstrong merged commit 99baca4 into paularmstrong:master Feb 9, 2017

jeffcarbs deleted the feature/immutable branch February 9, 2017 16:47

jeffcarbs mentioned this pull request Feb 9, 2017

Handle missing data in denormalize #232

Merged

3 tasks

lock bot added the Outdated label May 7, 2018

lock bot locked as resolved and limited conversation to collaborators May 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support denormalizing from immutable entities #228

Support denormalizing from immutable entities #228

jeffcarbs commented Jan 25, 2017 •

edited

Loading

coveralls commented Jan 25, 2017

jeffcarbs Jan 25, 2017

paularmstrong commented Jan 26, 2017

paularmstrong left a comment

paularmstrong Jan 30, 2017

paularmstrong Jan 30, 2017

paularmstrong Jan 30, 2017

paularmstrong Jan 30, 2017

paularmstrong Jan 30, 2017

coveralls commented Feb 3, 2017

coveralls commented Feb 3, 2017

jeffcarbs commented Feb 3, 2017 •

edited

Loading

coveralls commented Feb 3, 2017

paularmstrong left a comment •

edited

Loading

paularmstrong Feb 3, 2017

paularmstrong Feb 3, 2017

paularmstrong Feb 3, 2017

jeffcarbs commented Feb 3, 2017

coveralls commented Feb 3, 2017

jeffcarbs commented Feb 9, 2017

paularmstrong left a comment •

edited

Loading

paularmstrong Feb 9, 2017 •

edited

Loading

coveralls commented Feb 9, 2017

jeffcarbs commented Feb 9, 2017

jeffcarbs Feb 9, 2017

paularmstrong Feb 9, 2017

jeffcarbs Feb 9, 2017 •

edited

Loading

paularmstrong Feb 9, 2017

paularmstrong Feb 9, 2017

paularmstrong Feb 9, 2017

paularmstrong commented Feb 9, 2017

lock bot commented May 7, 2018

		@@ -1,3 +1,5 @@
		import { getIn, hasIn, isImmutable, setIn } from './ImmutableUtils';

Support denormalizing from immutable entities #228

Support denormalizing from immutable entities #228

Conversation

jeffcarbs commented Jan 25, 2017 • edited Loading

Problem

Solution

TODO

Size Diff

Before

After

coveralls commented Jan 25, 2017

Choose a reason for hiding this comment

paularmstrong commented Jan 26, 2017

paularmstrong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 3, 2017

coveralls commented Feb 3, 2017

jeffcarbs commented Feb 3, 2017 • edited Loading

coveralls commented Feb 3, 2017

paularmstrong left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs commented Feb 3, 2017

coveralls commented Feb 3, 2017

jeffcarbs commented Feb 9, 2017

paularmstrong left a comment • edited Loading

Choose a reason for hiding this comment

paularmstrong Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

coveralls commented Feb 9, 2017

jeffcarbs commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paularmstrong commented Feb 9, 2017

lock bot commented May 7, 2018

jeffcarbs commented Jan 25, 2017 •

edited

Loading

jeffcarbs commented Feb 3, 2017 •

edited

Loading

paularmstrong left a comment •

edited

Loading

paularmstrong left a comment •

edited

Loading

paularmstrong Feb 9, 2017 •

edited

Loading

jeffcarbs Feb 9, 2017 •

edited

Loading