Removes undefined values when populating

[pdokas]

Factory-girl does not support being passed an object spec where a member has the value undefined. This happens:

$ npm test -- test/utils/asyncPopulateSpec.js 

> [email protected] test /Users/pdokas/dev/factory-girl
> NODE_ENV=test mocha "test/utils/asyncPopulateSpec.js"



  asyncPopulate
    ✓ returns a promise
    ✓ throws error if target or source is not an object
    1) populates objects correctly
    ✓ overrides only provided data


  3 passing (28ms)
  1 failing

  1) asyncPopulate populates objects correctly:
     TypeError: Cannot convert undefined or null to object
      at getPrototypeOf (<anonymous>)
      at isPlainObject (src/utils/asyncPopulate.js:35:10)
      at src/utils/asyncPopulate.js:19:16
      at Array.map (<anonymous>)
      at asyncPopulate (src/utils/asyncPopulate.js:10:40)
      at _callee3$ (test/utils/asyncPopulateSpec.js:62:11)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:296:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
      at node_modules/babel-runtime/helpers/asyncToGenerator.js:35:14
      at new Promise (<anonymous>)
      at new F (node_modules/core-js/library/modules/_export.js:35:28)
      at node_modules/babel-runtime/helpers/asyncToGenerator.js:14:12
      at _callee$ (test/test-helper/asyncFunction.js:4:11)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:296:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
      at node_modules/babel-runtime/helpers/asyncToGenerator.js:35:14
      at new Promise (<anonymous>)
      at new F (node_modules/core-js/library/modules/_export.js:35:28)
      at Context.<anonymous> (node_modules/babel-runtime/helpers/asyncToGenerator.js:14:12)
      at Context.<anonymous> (test/test-helper/asyncFunction.js:2:23)

If source[attr] is defined as undefined, then asyncPopulate calls isPlainObject which passes undefined to Object.getPrototypeOf which throws a TypeError.

This PR alleviates this by having factory-girl remove members where the value is undefined. A use case where this comes up is when using factory-girl with structure. Structure validates objects against a spec, and allows you to define default values for individual members.

So if I have an object spec and a factory for it like this:

module.exports = structure({
  is_followed: {
    type: Boolean,
    default: false,
  }
});

factory.define('person', exports, {
  is_followed: factory.chance('bool'),
});

This is great for tests, except when you want to test that the default works because there is no way to tell factory-girl "completely leave this one value out". And so, this PR alleviates this need by allowing someone to build a factory and tell it to drop a member from the built object.

[pdokas]

P.S. I would love to document this in the readme, but I don’t see an obvious place to add the info. I’m absolutely open to suggestions!

[pdokas]

@aexmachina: Hi there, I don't mean to pester but I was wondering if you have any thoughts about this. Thank you!

[simonexmachina]

Thanks for this, sorry for the late reply!

[simonexmachina]

Why do we delete this? I would've thought it'd be better to just pass through the undefined to the target.

[pdokas]

The primary motivation here is to provide developers with a way of telling factory-girl to leave a particular attribute of the built object out. Without this all attributes are always required.

[simonexmachina]

I'm against this in principle, unless there's a compelling reason why. Factories define a particular "type" of object, which should be stable at runtime. If you do need to omit a property at runtime then you can use afterBuild or some other hook to delete the property.

I'm still happy for this PR, because we want to avoid getPrototypeOf(undefined) throwing an exception, but I don't think we should delete any of the properties 👍

[pdokas]

Hm, yeah, I see what you mean. That will probably work. I’ll give it a shot and report back.

Supposing passing through the value as undefined and then doing extra processing on it in afterBuild (in my case, deleting the key/value pair), would a diff like this be good to go?

+    } else if (typeof source[attr] === 'undefined') {
+      target[attr] = undefined;