-
Notifications
You must be signed in to change notification settings - Fork 376
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
Conversation
I still think that nobody will pick this up outside of the core users, as we've not made |
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. |
Btw, I don't actually understand what use case we break with this. @SimonRichardson could you elaborate? |
👍 Thanks for opening this pull request, @rpominov. :) |
👍 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. |
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. 👍 |
@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 |
@SimonRichardson I think prefixes will help get FL support in Lodash |
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 I think having a plain string of As long as the spec remains clear on the chosen methods, is there still a benefit in actually maintaining the |
@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 And seeing as prefixing is a library concern only, being terse doesn't carry much benefit for end users. |
The symbols would have to be registered in the global registry via As for naming things, |
Apologies, @scott-christopher I wasn't aware of
Completely agree ^^ |
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 |
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. |
Should I make change in PR |
I think the substring |
I have an idea: use |
What's the advantage of a symbol, @isiahmeadows? So far no one has been able to provide one. |
@davidchambers I think the main reason was to still allow people to use their own |
I really don't see the point of symbols for us at this moment in time. |
Yes. Well-chosen strings would do this just as well as symbols, but without the downsides. ;) |
@joneshf I think we're waiting for your approval on this. What are your thoughts? |
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. |
Sounds more like yea than nay 😄 |
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.
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). |
Let's merge this. The upside is significant; the downside negligible. |
Can you update to the latest master and then I'll merge it after that 😱 |
Awesome 🎉 |
⚡ |
Great! Shall we publish v1.0.0 now? I'm keen to update various libraries. :) |
* 'master' of github.com:fantasyland/fantasy-land: change spec to require prefixed names (fantasyland#146)
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. |
I'd like to see one of 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. |
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 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). |
I agree, and although I didn't voice it, I was thinking the same thing,
I agree. And I'd like to clarify that neither suggestion is exactly
Lol :-D (I think they're almost to 60 at this point.) On Tue, Sep 6, 2016, 12:25 David Chambers [email protected] wrote:
|
Yeah. But we also changing methods names now. So If for some reason we need to support old version, But I agree that we shouldn't wait too long. Maybe #135 ? |
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 Thanks! (I was planning on referencing it here, anyways.) |
@SimonRichardson What are your thoughts on merging #145 plus maybe #135 and releasing v1.0? |
Agreed, slowly getting there! |
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