evilsoft/crocks

Apply algebra doesn't dispatch correctly to fantasy-land

branneman opened this issue · 6 comments

branneman commented

Describing the bug in terms of the implementation:

To Reproduce

const fl = require('fantasy-land')
const R = require('ramda')
const C = require('crocks')
const inspect = require('util').inspect.custom

class Just {
  constructor(x) {
    this.x = x
  }
  static of(x) {
    return new Just(x)
  }
  [fl.map](f) {
    return Just.of(f(this.x))
  }
  [fl.chain](f) {
    return f(this.x)
  }
  [fl.ap](m) {
    return m[fl.map](this.x)
  }
  [inspect]() {
    return `Just ${this.x[inspect] ? this.x[inspect]() : this.x}`
  }
}

console.log(R.map(R.add(5), Just.of(5)))
console.log(R.chain(x => Just.of(R.add(5, x)), Just.of(10)))

const add5 = Just.of(R.add(5))

// Manual `ap` call works:
console.log(add5[fl.ap](Just.of(15)))

// Ramda's `ap` works:
console.log(R.ap(Just.of(20))(add5))

// Crocks' `ap` FAILS:
console.log(C.ap(Just.of(25))(add5))

Expected output

Just 10
Just 15
Just 20
Just 25
Just 30

Actual output

Just 10
Just 15
Just 20
Just 25

TypeError: ap: Both arguments must be Applys of the same type
    at ap (<redacted>/node_modules/crocks/pointfree/ap.js:14:11)
    at <redacted>/node_modules/crocks/core/curry.js:25:12
    at Object.<anonymous> (<redacted>/test-ap-crocks.js:43:30)

Possible solution

I implemented a quick hack to fix it:

Change src/pointfree/ap.js:20 to:

return (m[fl.ap] || m.ap).call(x, m)

Add the ap property to src/core/flNames.js:

ap: 'fantasy-land/ap'

Actual solution

Instead of maintaining a list of fantasy-land names in flNames.js, why not add the npm package fantasy-land as a dependency? It actually exports this list of names. I bet more stuff is missing, or will be in the near future ;)

evilsoft commented

Awesome. We have an issue for starting work on this here.
It is tricky, because we want to allow ap to behave with the function in the instance (opposite of what fl does).

We also need to make adjustments to each Apply type to account for that, as well as update traverse in some places.

We may be removing fantasy-land entirely, but will post up an issue for the community to debate the direction.

As far as dependencies got, we do not want to be tied to dependencies, especially around fantasy-land. That is why we do not use things like fantasy-land or fantasy-laws. If they pull something like they did with ap in the first place again, I do not want to be left behind if we opt not to go that way. So for instance, if a new property is added after a major changes, if we decide to stay up to our old version, but still want to include the new type, we can.

Will keep this open to track the work after applyTo is added to the types.

evilsoft commented

But. We may be able to just update the point free function, and worry about the fl dispatch on the crocks types for now, will need to think that trough on what the transition will be like. Also this will affect the lift functions, so you can verify by using liftA2 in crocks against your Just.

evilsoft commented

Ohhh. also, I think that implementation of fantasy-land/ap is backwards. The function is coming in as the data, that should be the value. To match the fantasy-land spec the implementation should be:

class Just {
  [fl.ap](m) {
    return this[fl.map](m.x)
  }
}

// These are now:
const add5 = Just.of(R.add(5))

// Manual `ap` call works:
console.log(Just.of(15)[fl.ap](add5))

// Ramda's `ap` works:
console.log(R.ap(add5)(Just.of(20)))

// Crocks' `ap` FAILS:
console.log(C.ap(add5)(Just.of(25)))

This comes from the Appy spec, so with those pointfree functions the data is the last, so that should be the value to be applied.

b must be an Apply of a function

branneman commented

also, I think that implementation of fantasy-land/ap is backwards

You're totally right, I was struggling a bit with that. I'm still learning about algebraic programming. Thanks for the tip!

Incidentally, it's also the reason I couldn't get Sanctuary's ap to play along in my example. Now it does:

const R = require('ramda')
const S = require('sanctuary')
const C = require('crocks')

class Just {
  constructor(x) {
    this.x = x
  }
  static of(x) {
    return new Just(x)
  }
  [fl.map](f) {
    return Just.of(f(this.x))
  }
  [fl.ap](m) {
    return this[fl.map](m.x)
  }
  [fl.chain](f) {
    return f(this.x)
  }
  [inspect]() {
    return `Just ${this.x[inspect] ? this.x[inspect]() : this.x}`
  }
}

// Manual `ap` call
console.log(Just.of(15)[fl.ap](add5))
//=> Just 20

// Ramda's `ap`
console.log(R.ap(add5)(Just.of(20)))
//=> Just 25

// Sanctuary's `ap`
console.log(S.ap(add5)(Just.of(30)))
//=> Just 35

// Crocks' `ap`
console.log(C.ap(add5)(Just.of(35)))
//=> Error ap: Both arguments must be Applys of the same type
branneman commented

As far as dependencies got, we do not want to be tied to dependencies, especially around fantasy-land. That is why we do not use things like fantasy-land or fantasy-laws. If they pull something like they did with ap in the first place again, I do not want to be left behind if we opt not to go that way. So for instance, if a new property is added after a major changes, if we decide to stay up to our old version, but still want to include the new type, we can.

You could choose to pin the version when you add fantasy-land as a dependency. That way you can choose to support a specific version of fantasy-land, and if you don't agree with the direction in the future, just don't support that newer version and don't upgrade. I believe other libraries are doing something similar.

Awesome. We have an issue for starting work on this here.

Will keep this open to track the work after applyTo is added to the types.

Ok sure, will track this!

evilsoft commented

You could choose to pin the version when you add fantasy-land as a dependency. That way you can choose to support a specific version of fantasy-land, and if you don't agree with the direction in the future, just don't support that newer version and don't upgrade. I believe other libraries are doing something similar.

We would have to add "new" things ourselves and curate it anyway. So for instance, invert for Group happened after 1.x so if we locked down pre-1.x, then we would have to curate invert anyway.