origamitower / folktale

[not actively maintained!] A standard library for functional programming in JavaScript
https://folktale.origamitower.com/
MIT License
2.04k stars 102 forks source link

Using lodash isPlainObject on a union instance throws an exception #190

Closed ohana54 closed 10 months ago

ohana54 commented 6 years ago

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

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

robotlolita commented 6 years ago

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 :/

ohana54 commented 6 years ago

@robotlolita that's great, thanks!