feat(data-point): Normalize collection and hash type checking functionality #214

raingerber · 2018-02-14T20:09:30Z

closes #189

What: always use outputType for type checking hash and collection entities (before, they had custom validate functions)

Why: type checking is always done in the same place now, whether or not the user has provided a custom type check reducer

How:

Refactored getTypeModifier to now process arrays (and renamed it to normalizeTypeCheckSource)
hash#outputType always uses the isObject reducer, following by an optional custom reducer
collection#outputType always uses the isArray reducer, following by an optional custom reducer
Throw error when trying to use an invalid outputType for hash or collection (like string, number, etc)
Reword the error message when type checking fails

Checklist:

Has Breaking changes
Documentation
Tests
Ready to be merged

…process arrays rename getTypeModifier to normalizeTypeCheckSource

…llection entities ViacomInc#189

…ng fails

raingerber · 2018-02-14T20:14:36Z

packages/data-point/lib/entity-types/base-entity/factory.js

+ */
+function normalizeTypeCheckSource (source) {
+  if (Array.isArray(source)) {
+    return source.map(r => normalizeTypeCheckSource(r))


hash and collection entities now pass arrays to this function

raingerber · 2018-02-14T20:15:02Z

packages/data-point/lib/entity-types/base-entity/resolve.test.js

@@ -146,17 +146,17 @@ describe('ResolveEntity.resolveEntity', () => {
  }

  test('It should resolve entity', () => {
-    return resolveEntity('hash:asIs', 'foo').then(acc => {
+    return resolveEntity('model:asIs', 'foo').then(acc => {


using model:asIs instead of hash:asIs because these entities are not receiving objects

raingerber · 2018-02-14T20:16:24Z

packages/data-point/lib/helpers/type-check-function-reducers.js

@@ -2,11 +2,14 @@ const Util = require('util')
 const _ = require('lodash')
 const utils = require('../utils')

-function errorMessage (value, expectedType) {
-  const entityId = _.get(value, 'reducer.spec.id', 'value')


I'm not sure if this line was needed, but it looks like it expects value to be an accumulator, which is not the case

coveralls · 2018-02-14T20:16:31Z

Coverage remained the same at 100.0% when pulling c673228 on raingerber:normalize-type-checking into e3a03c3 on ViacomInc:master.

coveralls · 2018-02-14T20:16:31Z

Coverage remained the same at 100.0% when pulling d760652 on raingerber:normalize-type-checking into a366091 on ViacomInc:master.

n

raingerber · 2018-02-15T18:05:26Z

I want to make a few changes to this, so I'm closing it for now.

…directory

…ecking

paulmolluzzo · 2018-02-15T17:40:43Z

packages/data-point/README.md

@@ -2045,7 +2046,7 @@ If `params.inspect` is a `function`, you may execute custom debugging code to be

 A Hash entity transforms a _Hash_ like data structure. It enables you to manipulate the keys within a Hash. 

-To prevent unexpected results, **Hash** can only process **Plain Objects**, which are objects created by the Object constructor. If [Hash.value](#hash-value) does not resolve to a Plain Object it will **throw** an error. 
+To prevent unexpected results, a **Hash** can only return **Plain Objects**, which are objects created by the Object constructor. If a hash resolves to a different type, it will throw an error. This type check occurs *before* the value is passed to the (optional) `outputType` reducer.


This rewording lost an important point: Hashes only process plain objects. Can we put that line back in somehow. (something like, "... a Hash can only accept and return Plain Objects...")

I just double checked and hash can accept any type - it's only required to return an object, which is why I changed this wording.

with that said, do you think that hash should only accept objects as input? that might be a good idea

Hmm, this test looks like it throws if the input to a hash is an array. Am I missing something?

https://github.com/ViacomInc/data-point/blob/master/packages/data-point/lib/entity-types/entity-hash/resolve.test.js#L33-L42

I think that's throwing because the output is an array, not because of the input (I think the name of the test is a little misleading though, since it makes it sound like the input needs to be an array)

Maybe we can clean up that test in this PR too then? I always thought the input needed to be an object. The README and the test both read that way for me.

👍 on the rewording as is.

Regarding ensuring the input is an object, that might be better off in a separate PR. This is mostly refactoring the outputType, and that would introduce changes to a different lifecycle method. What do you think?

sounds good, I do think we should make that change at some point though. I just checked and made sure the wording for the tests is updated in this PR (so it no longer has messages like entity.hash - only process Plain Objects)

paulmolluzzo · 2018-02-15T17:41:58Z

packages/data-point/README.md

@@ -2383,7 +2382,7 @@ A Collection entity enables you to operate over an array. Its API provides basic

 Collection entities expose a set of reducers that you may apply to them: [map](#collection-map), [find](#collection-find), [filter](#collection-filter). These reducers are executed in a [specific order](#collection-reducers-order). If you want to have more control over the order of execution, use the [compose](#compose-reducer) reducer.

-To prevent unexpected results, a **Collection Entity** can only process **Arrays**, if Collection.value does not resolve to an Array it will **throw** an error. 
+To prevent unexpected results, a **Collection** can only return arrays. If a collection resolves to a different type, it will throw an eror. This type check occurs *before* the value is passed to the (optional) `outputType` reducer.


Similar to my comment on Hash, Collections accept and return arrays.

paulmolluzzo · 2018-02-15T17:46:08Z

packages/data-point/lib/entity-types/base-entity/resolve.test.js

      expect(acc).toHaveProperty('value', 'foo')
    })
  })

  test('It should attach entityId to error', () => {
    const rejectResolver = () => Promise.reject(new Error('test'))
-    return resolveEntity('hash:asIs', undefined, undefined, rejectResolver)
+    return resolveEntity('model:asIs', undefined, undefined, rejectResolver)


Would this not work with the hash:asIs?

you're right, it works either way. changed it back to hash:asIs

… invalid outputTypes

…to type-check-functions.js

paulmolluzzo

LGTM! Didn’t realize some things about these entity types before the README updates. 😅

paulmolluzzo · 2018-02-16T15:08:18Z

packages/data-point/lib/entity-types/entity-hash/resolve.test.js

        expect(result).toMatchSnapshot()
      })
+  }
+  test('should throw error from default outputType reducer when input is not valid', () => {


Shouldn't this be should throw error from default outputType reducer when output is not valid?

good catch - fixed this along with a similar mistake in entity-collection/resolve.test.js

paulmolluzzo

Nice! 👍

Armstrong added 3 commits February 14, 2018 13:54

feat(data-point/base-entity): Rename getTypeModifier and allow it to …

d00a4e2

…process arrays rename getTypeModifier to normalizeTypeCheckSource

feat(data-point/entity-types): Add default outputTypes to hash and co…

cc37d06

…llection entities ViacomInc#189

test(data-point/helpers): Rephrase the error message when type checki…

c673228

…ng fails

raingerber self-assigned this Feb 14, 2018

raingerber commented Feb 14, 2018

View reviewed changes

docs(data-point/readme): Improve documentation for entity type checking

df357e6

n

raingerber closed this Feb 15, 2018

Matthew Armstrong added 2 commits February 15, 2018 13:06

refactor(data-point): Centralize type check functions in the helpers …

8580891

…directory

Merge remote-tracking branch 'upstream/master' into normalize-type-ch…

7fdac9f

…ecking

paulmolluzzo reviewed Feb 15, 2018

View reviewed changes

Matthew Armstrong added 2 commits February 15, 2018 17:01

feat(data-point/entity-types): Prevent hash and collection from using…

d6a10ff

… invalid outputTypes

refactor(data-point/helpers): Rename type-check-function-reducers.js …

c1daef8

…to type-check-functions.js

raingerber reopened this Feb 15, 2018

Matthew Armstrong added 2 commits February 15, 2018 17:09

test(base-entity/resolve.test): Replace model:asIs with hash:asIs

edc4e49

docs(data-point/readme): Remove an apostrophe

883a00c

paulmolluzzo previously approved these changes Feb 15, 2018

View reviewed changes

paulmolluzzo reviewed Feb 16, 2018

View reviewed changes

test(data-point/entity-types): Fix wording of a few test messages

58cd59d

raingerber dismissed paulmolluzzo’s stale review via 58cd59d February 16, 2018 15:49

paulmolluzzo approved these changes Feb 16, 2018

View reviewed changes

raingerber mentioned this pull request Feb 19, 2018

Use default type checking for hash and collection entity input #221

Closed

acatl approved these changes Feb 20, 2018

View reviewed changes

Merge branch 'master' into normalize-type-checking

d760652

acatl merged commit d083cd1 into ViacomInc:master Feb 20, 2018

raingerber deleted the normalize-type-checking branch February 26, 2018 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(data-point): Normalize collection and hash type checking functionality #214

feat(data-point): Normalize collection and hash type checking functionality #214

raingerber commented Feb 14, 2018 •

edited

raingerber Feb 14, 2018

raingerber Feb 14, 2018 •

edited

raingerber Feb 14, 2018

coveralls commented Feb 14, 2018

coveralls commented Feb 14, 2018 •

edited

raingerber commented Feb 15, 2018 •

edited

paulmolluzzo Feb 15, 2018

raingerber Feb 15, 2018 •

edited

paulmolluzzo Feb 15, 2018

raingerber Feb 15, 2018 •

edited

paulmolluzzo Feb 15, 2018

raingerber Feb 15, 2018

paulmolluzzo Feb 15, 2018

paulmolluzzo Feb 15, 2018

raingerber Feb 15, 2018

paulmolluzzo left a comment

paulmolluzzo Feb 16, 2018

raingerber Feb 16, 2018

paulmolluzzo left a comment

feat(data-point): Normalize collection and hash type checking functionality #214

feat(data-point): Normalize collection and hash type checking functionality #214

Conversation

raingerber commented Feb 14, 2018 • edited

Choose a reason for hiding this comment

raingerber Feb 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 14, 2018

coveralls commented Feb 14, 2018 • edited

raingerber commented Feb 15, 2018 • edited

Choose a reason for hiding this comment

raingerber Feb 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raingerber Feb 15, 2018 • edited

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

paulmolluzzo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmolluzzo left a comment

Choose a reason for hiding this comment

raingerber commented Feb 14, 2018 •

edited

raingerber Feb 14, 2018 •

edited

coveralls commented Feb 14, 2018 •

edited

raingerber commented Feb 15, 2018 •

edited

raingerber Feb 15, 2018 •

edited

raingerber Feb 15, 2018 •

edited