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

Flip arguments order in ap #145

Merged
merged 1 commit into from
Sep 13, 2016
Merged

Conversation

rpominov
Copy link
Member

@rpominov rpominov commented Jul 26, 2016

See #50 and #144

This is a breaking change and we have two options how to do it:

  1. Like any other changes that were done before (except they weren't breaking)
  2. Add prefixes to method names (Namespaced prefixes — next step for resolving name clashes #122) and bump major version number

If we go with the first option there will be a period when some libraries already updated but some not, so they might not work well with each other. For example a code like this may stop working:

import R from 'ramda'
import Foo from 'foo'

a = R.lift(x => y => x + y, Foo.of(1), Foo.of(2))

In this case users of libraries will have to freeze their dependencies' versions until all libs are updated.

If we do the second, a user of the libraries will be able to force one that already updated to work in v0.2 mode, by installing [email protected] instead of [email protected]. But the library that updated will need to support this too, by doing something like:

import fl from 'fantasy-land'

const lift2 = fl.version // supposedly we add .version property in 1.0
  ? (fn, x, y) => x[fl.map](x => y => fn(x, y))[fl.ap](y)
  : (fn, x, y) => y[fl.ap](x[fl.map](x => y => fn(x, y)))

export default lift2

Currently PR implements the first option, and I'll be happy to update it to the second if we decide so. And I, for one, think that we should go with the second option.

Whatever we decide, I think we should wait for some time before merging so everybody will have a chance to discuss.

@SimonRichardson
Copy link
Member

👍

@SimonRichardson
Copy link
Member

We should try and merge this one in tbh!

@joneshf thoughts?

@scott-christopher
Copy link
Contributor

scott-christopher commented Aug 5, 2016

Does your second option require all FL types in scope to implement the same version?

The only way I can see this landing without causing pretty major headaches would be to include a version in the method namespace, such as:

if ('fl@2/ap' in someApply) {
  // handle swapped arg order
} else {
  // handle legacy arg order
}

@rpominov
Copy link
Member Author

rpominov commented Aug 5, 2016

I was thinking about separate name for flipped ap too, this is an options yes. Basically we can do detection as 'fl@2/ap' in someApply or as fl.version > 1 (or just fl.version if we add version only starting from 1.0).

But actually all this solutions seem so complicated, maybe it's indeed better to just break some code for some period of time. It is hard for me to estimate amount of trouble this can make. I, for one, maintain very little of code that will need to be updated. So for me it's not a big issue. But I guess some people would rather do something like second option because of bigger amount of code they maintain.

@scott-christopher
Copy link
Contributor

scott-christopher commented Aug 5, 2016

If we were to go with the const { version } = require('fantasy-land') approach and stick with keeping it as a peer dependency, wouldn't that require both the implementers of the spec and consumers of the spec to handle the different version ranges (unless I've misunderstood)?

e.g.

const { ap, version } = require('fantasy-land')

Id.prototype[ap] = version >= 2 ? (other) => new Id(other.value(this.value))
                                : (other) => new Id(this.value(other.value))

//:: Apply f => f (a -> b) -> f a -> f b
const ap = (ap1, ap2) => version >= 2 ? ap2[ap](ap1)
                                      : ap1[ap](ap2)

Personally I'm not so sure that this change offers enough to warrant the break in backwards compatibility and added complexity.

If we can come up with a nicer way of managing interop between versions then I'll be a little more convinced. :)

@joneshf
Copy link
Member

joneshf commented Aug 5, 2016

We should try and merge this one in tbh!

@joneshf thoughts?

I'm almost definitely a YES! This has always confused me. Let me read this more thoroughly first though. :)

@rpominov
Copy link
Member Author

rpominov commented Aug 5, 2016

@scott-christopher Yea, both implementers and consumers would need to do the detection. Pretty complicated stuff, I agree.

@SimonRichardson
Copy link
Member

SimonRichardson commented Aug 5, 2016

I'd rather make this a breaking change, that's why we implemented semver. So I personally don't mind bumping to 1.0.0, that's what it's there for.

@davidchambers
Copy link
Member

If we were to go with the const { version } = require('fantasy-land') approach and stick with keeping it as a peer dependency, wouldn't that require both the implementers of the spec and consumers of the spec to handle the different version ranges (unless I've misunderstood)?

It wouldn't require this, as far as I can tell. The implementor of the Id data type could release a version which is only guaranteed to work with [email protected] (supporting older versions as well would be quite confusing, I think). Authors of libraries which provide functions that operate on values of FL-compatible data types have the choice of supporting both 0.3.x and 1.x.x.


Side note: In Node, at least, there's already a way to access the package's version:

require('fantasy-land/package.json').version

@SimonRichardson
Copy link
Member

Authors of libraries which provide functions that operate on values of FL-compatible data types have the choice of supporting both 0.3.x and 1.x.x.

Bingo!

@scott-christopher
Copy link
Contributor

I guess I'm not so concerned whether this is a backwards breaking change as much as there is a sane way for users and libraries interacting with FL types to manage these breaking changes going forward.

Authors of libraries which provide functions that operate on values of FL-compatible data types have the choice of supporting both 0.3.x and 1.x.x.

I apologise for not following along here, but could someone spell out how a library could support one type that implements the 0.3.x version of ap and another that supports 1.x at the same time? Or is there a constraint imposed on the user of such a library to pick one version that all types and supporting libraries must support in their application?

@SimonRichardson
Copy link
Member

could someone spell out how a library could support one type that implements the 0.3.x version of ap and another that supports 1.x at the same time

To put it bluntly we can't, but we shouldn't leave things broken just because of that.

@davidchambers
Copy link
Member

[C]ould someone spell out how a library could support one type that implements the 0.3.x version of ap and another that supports 1.x at the same time?

The library would need to provide two versions of each ap-related function, I think. In Ramda's case, the library could provide both R.ap and R._ap.

Or is there a constraint imposed on the user of such a library to pick one version that all types and supporting libraries must support in their application?

It needn't be all or nothing, but the burden would be on the user to know with which version of FL each of her data types is compatible, and choose—for example—R.ap or R._ap as appropriate.

This would, though, undermine one of the main benefits of FL: the ability to define functions which operate on many data types without specific knowledge of these data types.

If we're to make this change, I don't foresee a seamless transition. It seems the best approach would be to update the ap implementations for all the data types as quickly as possible, then release a new version of Ramda compatible with these updated data types.

@scott-christopher
Copy link
Contributor

It seems the best approach would be to update the ap implementations for all the data types as quickly as possible, then release a new version of Ramda compatible with these updated data types.

Right, but that's my concern. This change would introduce a state where libraries such as Ramda could no longer offer interoperability between types where some FL-implementing libraries update sooner than others. Given that interoperability is one of the main selling points of FL, I guess I'm failing to see how the current state is considered broken compared to the state that this could potentially introduce.

I may also be in the minority in thinking that there's nothing particularly wrong with the current spec, which may be influencing my thinking here. IMO, the existing spec feels familiar to the both Function.prototype.apply and <*>.

I don't want to stop movement on this if most people feel like this is the right decision. I'd just like to know that there is a way to manage the transition. If there is no easy way to manage the transition, I'd like to know that others feel this is important enough to cause the potential disruption.

@rpominov
Copy link
Member Author

rpominov commented Aug 5, 2016

Just to add two cents:

This change would introduce a state where libraries such as Ramda could no longer offer interoperability between types where some FL-implementing libraries update sooner than others.

Users could just stick with older versions for a period of time until all libraries they use together are updated.

I'd like to know that others feel this is important enough to cause the potential disruption.

To me the main selling point is better support for type systems like Flow #50 (comment)

@SimonRichardson
Copy link
Member

Given that interoperability is one of the main selling points of FL

For that version.

If we're never allowed to make breaking changes (ignoring adding items to the specification) then this project is dead! That means we can never learn from our mistakes, or removing dead weight and update the spec accordingly. The idea of "fantasy-land" is born from the fact that the promise specification wouldn't make the sacrifice of learning and updating when confronted with new knowledge.

@scott-christopher
Copy link
Contributor

Sure, like I said I don't have an issue with breaking changes, my point is that we look for ways of managing the transition and upgrade for those that depend on FL through the breaking changes, otherwise we're potentially just pushing the inability to move forward onto the users.

@SimonRichardson
Copy link
Member

Cool 👍

Either we should burden the library writers aka #145 (comment) or we expose ap' for sometime whilst people understand that a transition is being made.

```

- [`map`][] may be derived from [`chain`][] and [`of`][]:

```js
function(f) { var m = this; return m.chain(a => m.of(f(a))); }
function(f) { return this.chain(a => this.of(f(a))); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@joneshf
Copy link
Member

joneshf commented Aug 5, 2016

Alright. I'm good!

@SimonRichardson
Copy link
Member

Shall we merge this, I think we should.

@rpominov
Copy link
Member Author

rpominov commented Aug 8, 2016

If #146 will happen, would be great to release both of these PRs in the same version.

@SimonRichardson
Copy link
Member

I don't mind merging this, and holding the release until 146 lands.

@SimonRichardson
Copy link
Member

Considering #146 will be merged, can we update to the latest master please.

@rpominov
Copy link
Member Author

rpominov commented Sep 6, 2016

I'll update it after #146 lands if that's ok. It just would be easier to me as this and #146 also might have conflicts with each other.

@SimonRichardson
Copy link
Member

#146 has landed.

@rpominov
Copy link
Member Author

rpominov commented Sep 6, 2016

@SimonRichardson SimonRichardson merged commit db300ff into fantasyland:master Sep 13, 2016
@davidchambers
Copy link
Member

🎉

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

Successfully merging this pull request may close these issues.

5 participants