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

Change spec to require prefixed names #146

Merged
merged 1 commit into from
Sep 6, 2016

Conversation

rpominov
Copy link
Member

@rpominov rpominov commented Aug 6, 2016

This is an alternative, much simpler, plan for #122 . Instead of using a peer dependency approach, spec simply requires prefixed names.

This was briefly discussed in chat room https://gitter.im/fantasyland/fantasy-land?at=57a32d2fc915a0e426bbd412

Previous discussions: #92 #120 #122

@SimonRichardson
Copy link
Member

I still think that nobody will pick this up outside of the core users, as we've not made Type.prototype.map (etc) unusable. I not sure about prefixes at all, so we should put it to a vote, but mine is 👎

@rpominov
Copy link
Member Author

rpominov commented Aug 6, 2016

For me, personally, main purpose of the spec is to provide an interoperability base. For example library A provides some FL types, library B provides some generic code that can work with any FL compatible types, someone uses library B with library A. For this use case of the spec prefixes are essential, because library B may also support other interfaces beside FL, and needs to detect FL compatible values somehow.

I think people will pick this up and will use FL more because FL will support interoperability use case better.

@rpominov
Copy link
Member Author

rpominov commented Aug 6, 2016

Btw, I don't actually understand what use case we break with this. @SimonRichardson could you elaborate?

@davidchambers
Copy link
Member

👍

Thanks for opening this pull request, @rpominov. :)

@JAForbes
Copy link
Member

JAForbes commented Aug 8, 2016

👍 I think a symbol is overkill. I see real potential in more and more libraries supporting fantasy land, but having to import the library to get a symbol makes assumptions about the users of FL (e.g. they are on node, or they are using a bundler).

A lot of JS devs appreciate 0 deps. Requiring some runtime code insertion is surprising behaviour for a specification. It is unnecessary friction.

If we are prefixing, I suggest the prefix includes the complete github namespace'd repo address followed by the function name (e.g. fantasyland/fantasy-land:map instead of fantasyland/map). It leaves us with more room for future parallel specs and creates a replicable approach for other standards to adopt.

@scott-christopher
Copy link
Contributor

There is nothing to suggest that an implementing library must only expose methods via the specified FL names/symbols. They could still offer their friendly name equivalents for end users to access, but the namespace would also be available for other libraries to detect implementations without the risk of method name collisions. This should also avoid the end user from having to depend directly on the fantasy-land package.

For example:

// fantasy-land/index.js
module.exports = {
  //...
  map: Symbol.for('fl/map')
  //...
}
// some implementation
const fl = require('fantasy-land')

Identity.prototype.map =
Identity.prototype[fl.map] = function(f) {
  return new Identity(f(this.value))
}

// or

class Identity {
  //...
  map(f) { return new Identity(f(this.value)) }
  //...
}
Identity.prototype[fl.map] = Identity.prototype.map
// some utility library
const fl = require('fantasy-land')

const map = (f, fa) =>
  typeof fa[fl.map] === 'function'
    ? fa[fl.map](f)
    : // handle other built-in types, etc.

Symbols could be swapped out for strings in the above example if it is desirable to support legacy JS environments, but the general gist remains the same.

👍

@JAForbes
Copy link
Member

JAForbes commented Aug 8, 2016

@scott-christopher just to clarify, when I was talking about users, I was talking about implementers of the spec; library authors that want their library to have 0 dependencies and want to avoid bundling. I wasn't talking about users of libraries that implement FL.

I'm specifically thinking about Mithril's upcoming release which includes FL compatible Applicative Functors streams (inspired by flyd). The entire library is self contained, so implementing the prefix internally is much more likely than a symbol. I think other front end libraries would have a similar requirement/preference.

And I think having both symbols and prefixes is a mistake. It creates more runtime pressure on libraries like Ramda when dispatching. They need to check for symbol or a prefix - more moving parts makes for more fragile implementations.

If we are methodical about prefixing we solve the problem of duck typing, support more environments and remove the requirement for implementers to require('fantasy-land').

@JAForbes
Copy link
Member

JAForbes commented Aug 8, 2016

@SimonRichardson I think prefixes will help get FL support in Lodash

@scott-christopher
Copy link
Contributor

It sounds like there are two separate and somewhat orthogonal concerns here then: whether symbols provide any value for namespacing, and what can be done to remove the need for require('fantasy-land')?

I think having a plain string of "fl/map" rather than Symbol.for('fl/map') would reduce the friction for adoption while providing enough of a namespace to avoid co-incidental collisions, so I tend to agree that symbols would probably be an unnecessary overhead.

As long as the spec remains clear on the chosen methods, is there still a benefit in actually maintaining the fantasy-land/index.js exported method names?

@JAForbes
Copy link
Member

JAForbes commented Aug 8, 2016

@scott-christopher I assumed that in order to use a symbol you'd need to import the library in order to access it. So therefore they were related concerns. But maybe I'm misunderstanding symbols?

I can live with fl/map but I think it'd be nice to formalize prefixing. A prefix is just a namespace, and by making the prefix the same as the github project's namespace there is a certain level of authority when using that prefix for this project.

And seeing as prefixing is a library concern only, being terse doesn't carry much benefit for end users.

@scott-christopher
Copy link
Contributor

The symbols would have to be registered in the global registry via Symbol.for to overcome the shenanigans involved with crossing execution realms, meaning libraries could also just call Symbol.for('fantasy-land/map') in order to retrieve the symbol instance if so desired. The symbols do start to just feel like convoluted strings by this point though, especially if you're forced to retrieve them via a namespaced string anyway.

As for naming things, fantasy-land/map, fantasy-land/chain, etc seem fine to me.

@JAForbes
Copy link
Member

JAForbes commented Aug 8, 2016

Apologies, @scott-christopher I wasn't aware of Symbol.for.

The symbols do start to just feel like convoluted strings by this point though, especially if you're forced to retrieve them via a namespaced string anyway.

Completely agree ^^

@SimonRichardson
Copy link
Member

If we implemented the spec like @scott-christopher shows in this comment then I'm not against this proposal and I would want to show or mention that this could be a way for users of the spec to correctly implement things.

Although, I am against the usage of symbols for keys though, there has been a long discussion about this before (there was several reasons and I'll try and dig up the post/comment).

I don't care about the prefix too much, i.e. if it should contain @ or not, but namespacing it with fantasy-land does make sense.

@rpominov
Copy link
Member Author

rpominov commented Aug 8, 2016

Regarding Symbols. They're maybe great, but currently may cause too much trouble in legacy JS environments. Also I don't understand what advantage Symbols have for our use-case. I think we'll be able to switch to Symbols in the future if this will improve usability of FL somehow.

@rpominov
Copy link
Member Author

rpominov commented Aug 8, 2016

Should I make change in PR @fantasy-land/ -> fl/? I don't have a strong preference either way.

@SimonRichardson
Copy link
Member

SimonRichardson commented Aug 8, 2016

I totally agree with symbols, I see no advantage atm in time. Concerning namespace; I'd rather have fantasy-land/ because it's more explicit. fl still reminds me of flash.

And I don't even want to go there!

@davidchambers
Copy link
Member

I think the substring 'fantasy-land' should be present. It's the name of the package in the public npm registry, so it's a namespace this project "owns" (unlike fl, for example).

@dead-claudia
Copy link
Contributor

I have an idea: use Symbol.for, but if Symbol.for doesn't exist (or doesn't return an actual Symbol), then fall back to literal fantasy-land/* names, to avoid cross-realm issues.

@davidchambers
Copy link
Member

What's the advantage of a symbol, @isiahmeadows? So far no one has been able to provide one.

@dead-claudia
Copy link
Contributor

@davidchambers I think the main reason was to still allow people to use their own map or chain that isn't Fantasy Land-compatible, and still conform to the Fantasy Land API. I don't fully agree namespacing is even necessary in the first place, though.

@SimonRichardson
Copy link
Member

I really don't see the point of symbols for us at this moment in time.

@davidchambers
Copy link
Member

I think the main reason was to still allow people to use their own map or chain that isn't Fantasy Land-compatible

Yes. Well-chosen strings would do this just as well as symbols, but without the downsides. ;)

@rpominov
Copy link
Member Author

@joneshf I think we're waiting for your approval on this. What are your thoughts?

@rpominov rpominov mentioned this pull request Aug 21, 2016
@joneshf
Copy link
Member

joneshf commented Aug 21, 2016

Every time I come back to the namespaced discussion I forget the motivation and question why it's needed. Since it can't stick in my mind as a positive, I'm pretty much a 👎. However, since I can't seem to understand it for more than a few hours, most other people seem to get it, it doesn't seem bad, and I don't feel like I should be holding the change back, I'm also a 👍.

What I'm trying to say is, I abstain from voting.

Ultimately we're SemVer'd, merge it through, and if it turns out bad, we can revert with another version. But, I'd rather we make progress than stagnate.

@rpominov
Copy link
Member Author

Sounds more like yea than nay 😄
I'm also not 100% sure but I think we should try this. This have potential to turn out very good.

Avaq added a commit to fluture-js/Fluture that referenced this pull request Aug 30, 2016
Closes #19.

Chose not to use a peer dependency, but instead to define
the constants myself as specified by FantasyLand.

This method was suggested in
fantasyland/fantasy-land#146
and seems to be the generally accepted way of complying
to FantasyLand.
@rpominov
Copy link
Member Author

rpominov commented Sep 3, 2016

So what do we do? I think we should either merge this, or give up on prefixing ideas and roll back to how it was before (before #120 was merged).

@davidchambers
Copy link
Member

Let's merge this. The upside is significant; the downside negligible.

@SimonRichardson
Copy link
Member

Can you update to the latest master and then I'll merge it after that 😱

@rpominov
Copy link
Member Author

rpominov commented Sep 5, 2016

Awesome 🎉
Will update first thing tomorrow.

@rpominov
Copy link
Member Author

rpominov commented Sep 6, 2016

@SimonRichardson SimonRichardson merged commit 53e7504 into fantasyland:master Sep 6, 2016
@davidchambers
Copy link
Member

Great! Shall we publish v1.0.0 now? I'm keen to update various libraries. :)

rjmk added a commit to rjmk/fantasy-land that referenced this pull request Sep 6, 2016
* 'master' of github.com:fantasyland/fantasy-land:
  change spec to require prefixed names (fantasyland#146)
@rpominov
Copy link
Member Author

rpominov commented Sep 6, 2016

Yea, v1.0 including this and #145 would be great. Are there other breaking changes we may want to make? Because now is a perfect time.

@dead-claudia
Copy link
Contributor

I'd like to see one of Monoid.prototype.empty or Monoid.empty (and same with Applicative's of) to disappear. IMHO it just complicates handling FL types without any benefit.

Also, I'm thinking maybe create a helper function that creates a wrapper for all the methods, but curried, based on the type's constructor and one or more FL algebras, but I'm not going to push that one too hard.

@davidchambers
Copy link
Member

I'd like to see one of Monoid.prototype.empty or Monoid.empty […] disappear.

If we're to have only one of these (which I agree is a good idea) it must be the prototype method. That way it's possible to have Identity([1, 2, 3]).empty() dispatch to the inner value's empty method to give Identity([]). There's no way to support Identity.empty().

I don't think pending improvements such as this should prevent us from releasing a new version of the spec. We won't run out of version numbers (ask the Google Chrome team).

@dead-claudia
Copy link
Contributor

If we're to have only one of these (which I agree is a good idea)
it must be the prototype method.

I agree, and although I didn't voice it, I was thinking the same thing,
too. One source of truth is always better than two.

I don't think pending improvements such as this should prevent us from
releasing a new version of the spec.

I agree. And I'd like to clarify that neither suggestion is exactly
something I couldn't live without. It's more of a nice to have than a
requirement. Specifications usually are slow moving, anyways. The HTML
standard and CSS standards are by far the exception, not the rule. But
changing core features would still take years there, too, like with the
transition to HTML 5.

We won't run out of version numbers (ask the Google Chrome team).

Lol :-D (I think they're almost to 60 at this point.)

On Tue, Sep 6, 2016, 12:25 David Chambers notifications@github.com wrote:

I'd like to see one of Monoid.prototype.empty or Monoid.empty […]
disappear.

If we're to have only one of these (which I agree is a good idea) it
must be the prototype method. That way it's possible to have Identity([1,
2, 3]).empty() dispatch to the inner value's empty method to give
Identity([]). There's no way to support Identity.empty().

I don't think pending improvements such as this should prevent us from
releasing a new version of the spec. We won't run out of version numbers
(ask the Google Chrome team).


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#146 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AERrBB94FDHR6ejjrDWGOGn2PVveKzmAks5qnZP-gaJpZM4JeW8f
.

@rpominov
Copy link
Member Author

rpominov commented Sep 6, 2016

I don't think pending improvements such as this should prevent us from releasing a new version of the spec. We won't run out of version numbers (ask the Google Chrome team).

Yeah. But we also changing methods names now. So If for some reason we need to support old version, ap vs fatasy-land/ap is how we can tell what version of ap a value has.

But I agree that we shouldn't wait too long. Maybe #135 ?

@rpominov
Copy link
Member Author

rpominov commented Sep 6, 2016

Although #135 isn't exactly the same situation as with ap. So maybe we shouldn't wait.

@dead-claudia
Copy link
Contributor

I'll note that the tests are mostly red ATM because of the namespacing change. I'm looking into that currently, and will file a PR as soon as I fix it. That may be worth merging quickly.

@davidchambers
Copy link
Member

#157

@dead-claudia
Copy link
Contributor

@davidchambers Thanks! (I was planning on referencing it here, anyways.)

@rpominov
Copy link
Member Author

@SimonRichardson What are your thoughts on merging #145 plus maybe #135 and releasing v1.0?

@SimonRichardson
Copy link
Member

Agreed, slowly getting there!

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.

7 participants