Skip to content
This repository has been archived by the owner on Mar 20, 2022. It is now read-only.

Handle missing data in denormalize #232

Merged

Conversation

jeffcarbs
Copy link
Contributor

@jeffcarbs jeffcarbs commented Feb 9, 2017

Following from discussion here #228 (comment)

Problem

When an entity is not found during denormalization, it throws an unexpected exception.

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)

Solution

During denormalization, if an entity is not found return undefined in its place.

NOTE: Initially I returned null where missing (null or undefined) data was. I've updated it to handle missing data by returning whatever was there, respecting either null or undefined.

TODO

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

@@ -63,7 +63,7 @@ export default class EntitySchema {

denormalize(entityOrId, unvisit, getDenormalizedEntity) {
const entity = getDenormalizedEntity(this, entityOrId);
if (typeof entity !== 'object') {
if (typeof entity !== 'object' || entity === null) {
Copy link
Contributor Author

@jeffcarbs jeffcarbs Feb 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered a fun fact about javascript while debugging this: typeof null === 'object'

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 99.419% when pulling 4d43c07 on jcarbo:feature/handle-not-found into fcee566 on paularmstrong:master.

@jeffcarbs
Copy link
Contributor Author

jeffcarbs commented Feb 9, 2017

Note there's a similar PR that was recently closed: #229.

That PR returns null for the entire denormalize call if any data is missing, this PR will put undefined at the spot of the missing data. For example:

const foodSchema = new schema.Entity('foods');
const menuSchema = new schema.Entity('menus', {
  food: foodSchema
});

const entities = {
  menus: {
    1: { id: 1, food: 2 }
  },
  foods: {
    1: { id: 1 }
  }
};

// On PR #229
denormalize(1, mySchema, entities) === null
denormalize(2, mySchema, entities) === null

// On this PR
denormalize(1, mySchema, entities) === {
  "food": undefined,
  "id": 1,
}
denormalize(2, mySchema, entities) === undefined

@jeffcarbs jeffcarbs force-pushed the feature/handle-not-found branch from 4d43c07 to 88136e1 Compare February 9, 2017 18:28
@jeffcarbs jeffcarbs changed the title Denormalize missing data as null Handle missing data in denormalize Feb 9, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 88136e1 on jcarbo:feature/handle-not-found into fcee566 on paularmstrong:master.

@paularmstrong paularmstrong merged commit 88136e1 into paularmstrong:master Feb 9, 2017
@paularmstrong
Copy link
Owner

🎉 looks good to me!

@paularmstrong
Copy link
Owner

@jcarbo looks like you maybe don't check twitter anymore. Your changes have been published to v3.2.0

@jeffcarbs jeffcarbs deleted the feature/handle-not-found branch February 10, 2017 02:24
@jeffcarbs
Copy link
Contributor Author

Thanks for the quick review/merge!

@lock
Copy link

lock bot commented May 7, 2018

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.

@lock lock bot added the Outdated label May 7, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants