Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

in ap.js #2414

Open
imchenglibin opened this issue Dec 14, 2017 · 10 comments
Open

in ap.js #2414

imchenglibin opened this issue Dec 14, 2017 · 10 comments

Comments

@imchenglibin
Copy link

imchenglibin commented Dec 14, 2017

implement is like this:

var ap = _curry2(function ap(applyF, applyX) {
  return (
    
    typeof applyX['fantasy-land/ap'] === 'function' ?
      applyX['fantasy-land/ap'](applyF) :


    typeof applyF.ap === 'function' ?
      applyF.ap(applyX) :
    typeof applyF === 'function' ?
      function(x) { return applyF(x)(applyX(x)); } :
    // else
      _reduce(function(acc, f) { return _concat(acc, map(f, applyX)); }, [], applyF)
  );
});

not this:

var ap = _curry2(function ap(applyF, applyX) {
  return (

    typeof applyF['fantasy-land/ap'] === 'function' ?
      applyF['fantasy-land/ap'](applyX) :


    typeof applyF.ap === 'function' ?
      applyF.ap(applyX) :
    typeof applyF === 'function' ?
      function(x) { return applyF(x)(applyX(x)); } :
    // else
      _reduce(function(acc, f) { return _concat(acc, map(f, applyX)); }, [], applyF)
  );
});

why?

@CrossEye
Copy link
Member

When we try to match the FantasyLand specifications, it is more consistent with Ramda that the translation of FantasyLand methods put the receiving object as the last parameter. This is the FantasyLand signature:

ap :: Apply f => f a ~> f (a -> b) -> f b

(where the squiggly array (~>) represents method application on the object to its left.)

This translates to a Ramda pure function as

ap :: Apply f => f (a -> b) -> f a -> f b

So I think this is right. Anyone disagree?

@imchenglibin
Copy link
Author

function Container(value) {
    this.__value = value;
}

Container.of = function (value) {
    return new Container(value);
}

Container.prototype.map = function (fn) {
    return Container.of(fn(this.__value));
}

A. Implements 'fantasy-land/ap'

Container.prototype['fantasy-land/ap'] = function (otherContainer) {
    return otherContainer.map(this.__value);
}

usage:

ap(Container.of(3), map(add, Container.of(2)));  // Container(5)

B. Implements 'ap'

Container.prototype['ap'] = function (otherContainer) {
    return otherContainer.map(this.__value);
}

usage:

ap(map(add, Container.of(2)), Container.of(3));  // Container(5)

both A and B are ap functor, and actually they are the same ap functor, but usage are different(params order).

@CrossEye
Copy link
Member

Ok, I see. The delegations to ap and fantasy-land/ap disagree on parameter order. That seems pretty bad. I think that our FantasyLand one is correct, and we should fix the plain one. Do you agree?

@imchenglibin
Copy link
Author

Implements 'fantasy-land/ap'

function Container(value) {
    this.__value = value;
}

Container.of = function (value) {
    return new Container(value);
}

Container.prototype.map = function (fn) {
    return Container.of(fn(this.__value));
}

Container.prototype['fantasy-land/ap'] = function (applyF) {
    return this.map(applyF.__value);
}

usage:

ap(map(add, Container.of(2)), Container.of(3))

FantasyLand one is correct, and the delegation to ap is incorrect

@kedashoe
Copy link
Contributor

Ok, I see. The delegations to ap and fantasy-land/ap disagree on parameter order. That seems pretty bad. I think that our FantasyLand one is correct, and we should fix the plain one. Do you agree?

This was done intentionally, see #1900, #1900 (comment)

@imchenglibin
Copy link
Author

imchenglibin commented Dec 19, 2017

so i should implement ap functor as below?

Implements fantasy-land/ap

Container.prototype['fantasy-land/ap'] = function (applyF) {
    return this.map(applyF.__value);
}

implements ap

Container.prototype['ap'] = function (otherContainer) {
    return otherContainer.map(this.__value);
}

@CrossEye
Copy link
Member

Even with @kedashoe's prompting, I still cannot remember why we didn't switch the order of ap when we added fantasy-land/ap.

This from @davidchambers:

The order in which a fantasy-land/ap method takes its arguments and the order in which R.ap takes its arguments are unrelated concerns. The change to the FL spec was made to bring ap into line with map and chain.

Doesn't seem to add anything we should care about. Even if we could keep ap separate from fantasy-land/ap, why would we do it? I think I fell down on the job there, when that PR stayed open too long, and didn't push back on this. But I'm in favor of switching ap to match.

@davidchambers
Copy link
Member

From my perspective we only dispatch to ap for compatibility with old versions of Fantasy Land. For backwards compatibility, in other words. Changing the way in which we dispatch to ap would break backwards compatibility, at which point we could simply remove the fallback and dispatch to fantasy-land/ap exclusively.

@CrossEye
Copy link
Member

I'm sure you know that my perspective on dispatching is a bit different. 😈. But that's right, we kept that for backward compatibility not with our internal code but an older FL version.

Hmmm.

@customcommander
Copy link
Member

@CrossEye: if I understand this correctly I can confirm that this comment from @imchenglibin is still correct as of today:

  1. The FL implementation is correct (i.e ApplyX['fantasy-land/ap'](ApplyF))
  2. The delegation is incorrect (i.e. applyF.ap(applyX))

Our next v1.0 is likely to contain breaking changes anyway so we might as well address this issue? Otherwise let's close?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants