folktale icon indicating copy to clipboard operation
folktale copied to clipboard

Using lodash isPlainObject on a union instance throws an exception

Open ohana54 opened this issue 6 years ago • 2 comments

Hi, We encountered an issue with Union and method definitions, where running lodash.isPlainObject throws an exception on a union type. When declaring a union, we use the ES6 shorthand method declaration:

const TestUnion = union('TestUnion', {
    Nothing() { }
})

This means that TestUnion.Nothing does not have a constructor in the same way that a "regular" function has, which fails the following check in lodash: https://github.com/lodash/lodash/blob/4.17.10/lodash.js#L12044 This is according to spec, and is also described here, under the title "Method definitions are not constructable".

Note: this does not happen with the built-in union Maybe.

Our use case: We use lodash.merge function to gather application state from several sources into one object. Some of the state includes union instances. lodash.merge uses isPlainObject internally, thus failing the merge.

Steps to reproduce

This is the piece of code that reproduces the problem. There's also a runnable version here: https://runkit.com/ohana54/folktale-lodash-bug

const union = require('folktale/adt/union/union')
const isPlainObject = require('lodash/isPlainObject')

const TestUnion = union('TestUnion', {
    Working: function () { },
    NotWorking() { }  
})

// Works
isPlainObject(TestUnion.Working())

// Throws "TypeError: Function has non-object prototype 'undefined' in instanceof check"
isPlainObject(TestUnion.NotWorking())

Expected behaviour

Expected isPlainObject not to throw an error

Observed behaviour

isPlainObject Throws TypeError: Function has non-object prototype 'undefined' in instanceof check

Environment

  • Mac OSX
  • Node 8, Node 9
  • Folktale v2.1.0

Thank you for this awesome library, we appreciate your time creating and maintaining it :)

ohana54 avatar Apr 30 '18 05:04 ohana54

The fix has been merged on master. This will probably be a major release though. It's unlikely to break things, but previously the initialiser (NotWorking in this case) was exposed as the constructor for that variant. This was wrong, but there may be people relying on this bug so a major version would be the safe thing to do :/

robotlolita avatar Jun 30 '18 21:06 robotlolita

@robotlolita that's great, thanks!

ohana54 avatar Jul 02 '18 21:07 ohana54