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

Drop prefix from canonical property #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paldepind
Copy link

@paldepind paldepind commented Apr 30, 2018

This PR renames the canonical property from fantasy-land/canonical to just canonical.

As I mentioned in fantasyland#286 I think the prefix with a slash made more sense when there were many prefixed methods. By design, there is only a single canonical method. Having a "directory"-like prefix that is only ever going to have a single child seems a bit odd to me. It seems sufficient to just pick a name that is uncommon enough to no cause collisions. As far as I can tell canonical in itself does that job just fine.

Benefits:

  • Shorter name is nice.
  • The name doesn't contain special characters so the property can be accessed without brackets and quotes. I.e. thing.canonical instead of thing["fantasy-land/canonical"].
  • IMO fantasy-land/canonical gives the misleading impression that there are several functions behind the prefix (like there was previously).

@davidchambers
Copy link

I think fantasy-land would be a better name than canonical, as it's more descriptive.

The name doesn't contain special characters so the property can be accessed without brackets and quotes. I.e. thing.canonical instead of thing["fantasy-land/canonical"].

This is a minor benefit given that users should not need to access the property directly. I agree that it would be nice, but I would argue for fantasy-land over fantasyland because I believe matching the npm package name is more important than saving ADT authors a few keystrokes.

@gabejohnson
Copy link

David beat me to the punch. canonical introduces too great a risk of name clash IMO.

@rpominov
Copy link
Owner

As @gabejohnson mentioned we want to avoid name clashes. Plus forcing to use string literal seems to help Google Closure compiler to handle it right (was mentioned here fantasyland/static-land#45 (comment))

IMO fantasy-land/canonical gives the misleading impression that there are several functions behind the prefix (like there was previously).

In fact we may introduce other properties, see #3 for example.

@paldepind
Copy link
Author

I think fantasy-land would be a better name than canonical, as it's more descriptive.

I don't see how it is more descriptive? Fantasy Land is just the name of the specification? It seems more descriptive to call the canonical property canonical? But, we could also consider different names. Wouldn't something like instances be even more descriptive?

canonical introduces too great a risk of name clash IMO.

What libraries have a property called canonical? To me it doesn't seem like a word anyone would use.

To me, the most important thing is to drop the slash. I still think fantasy-land is a lot better than fantasy-land/canonical and if that's what the majority prefers I'll update the PR 😄 But, I do prefer canonical over fantasy-land. To me there is something messy about embedding an arbitrary name into the code that makes up the spec itself.

@davidchambers
Copy link

To me there is something messy about embedding an arbitrary name into the code that makes up the spec itself.

I don't consider fantasy-land an arbitrary name, as it matches the npm package name and the GitHub repository name.

@paldepind
Copy link
Author

@davidchambers

I don't consider fantasy-land an arbitrary name, as it matches the npm package name and the GitHub repository name.

Yes, I agree with that. You're definitely right that the name isn't arbitrary in that it very specifically mentions which specification it is part of. But, my point was more that the specification name Fantasy Land itself is arbitrary. The specification might as well have been called Lambda Land or Fantasy Forest. It doesn't say anything about what the specification or what the method is. But, a name like canonical or instances says something about what the property itself is. In general, it seems very unconventional to put the name of a library or a specification in the code itself.

@rpominov

In fact we may introduce other properties, see #3 for example.

I hadn't seen that. Maybe this approach #2 (comment) would avoid that extra complexity?

@davidchambers
Copy link

In general, it seems very unconventional to put the name of a library or a specification in the code itself.

True, but it seems to me a reasonable way to virtually eliminate the possibility of accidental collisions. Also, a web search for fantasy-land is likely to be more fruitful than a web search for canonical or instances.

@gabejohnson
Copy link

gabejohnson commented Apr 30, 2018

What libraries have a property called canonical? To me it doesn't seem like a word anyone would use.

I don't know of any, but I can imagine some library like JQuery using it as a flag on an object representing a <link> element. Or maybe some URL library.

Also, if we're making a proposal to use canonical, I could see some other organization deciding to use such a property (hopefully opting not to 😄).

@rpominov
Copy link
Owner

To me just canonical seems weird. For example, consider an object myStream which is some kind of stream. What is myStream.canonical? Canonical what? The most descriptive name would be myStream["fantasy-land canonical module"], which is too long, of course. I think it's ok to shorten it to just fantasy-land though.

@rpominov
Copy link
Owner

@paldepind

Also, not to argue for keeping /canonical postfix, but I don't understand how thunks related to fantasy-land/instances proposed in #3 .

@paldepind
Copy link
Author

@rpominov

To me just canonical seems weird. For example, consider an object myStream which is some kind of stream. What is myStream.canonical? Canonical what?

I can see what you mean. The word isn't actually very descriptive. What about instances instead?

@gabejohnson

I don't know of any, but I can imagine some library like JQuery using it as a flag on an object representing a <link> element. Or maybe some URL library.

I actually don't think that example is a problem at all given the new design. I think we should consider how the new spec design changes the "name clash problem". Back when the prefix originally was introduced the spec used a lot of very popular names like map and reduce. This had two negative consequences:

  1. It could make it tricky for libraries to adopt FL if they already had an incompatible method with the same name.
  2. It could be hard to figure out at run-time if a value actually implemented FL or just happened to have a map method.

However, the new spec only uses a single property name. Let's say that name was instances. Even if some library happened to use the name instances it wouldn't result in the two same problems as before.

  1. Demanding a single uncommon property is very reasonable and probably there is no single library that already has an instances property and which wants to implement FL.
  2. Even if some library has an instances property it is extremely unlikely that the property is also an object and that the object have one of the FL function names. I run-time check like typeof x.instances === 'object' && typeof x.instances.concat === 'function' is going to be very robust.

What I'm trying to say is that we shouldn't worry about name clashes due to reasons that were only relevant in the past. A name like instances is more descriptive about what the property actually is, it is shorter to type and it doesn't use special characters which makes the property easier to access.

@rpominov
Copy link
Owner

rpominov commented May 5, 2018

run-time check like typeof x.instances === 'object' && typeof x.instances.concat === 'function' is going to be very robust.

Wasn't the original idea to reduce the amount of code? 😉

@joshburgess
Copy link

joshburgess commented May 19, 2018

What do you all think about using an ES6 Symbol instead? It could live as a standalone npm package that people just install & import.

Something like:

export const SymbolFantasyLandModule = Symbol('Fantasy Land Module')

Inspiration: https://github.com/benlesh/symbol-observable for observables

You would never have to worry about accidental name clashes at all this way.

You could also alias it to instances or canonical or FL or FL_Module or whatever in your own library code if you prefer reading a different name, because at the end of the day the Symbol value is still the same.

@davidchambers
Copy link

What do you all think about using an ES6 Symbol instead?

I'm opposed to this option. It would increase the complexity of building and distributing ADT libraries for a negligible gain. A string is the pragmatic option. That's my view, at least. ;)

@rpominov
Copy link
Owner

Symbols are still relatively new, which causes two kinds of issues:

  1. Support in browsers and other tools (e.g. Flow type checker) may be not complete or don't exist at all.
  2. It's still new to developers. Which means best practices are not established yet, plus if # 1 cause issues they may be even more confusing and hard to solve.

Although I have almost zero experience with Symbols, and can't give examples of any particular issues. To me, this just looks like complex and new technology, and using it seems like a risk, especially for a glue layer that supposed to connect many libraries.

On the other hand, Symbols seem like a solution designed specifically for our problem and built-in into the language. This seems like a modern and idiomatic solution that any serious project should use. Perhaps if we adopt Symbols we may attract more developers.

That are my thoughts, but as I say I don't really consider myself an expert in this area.

@paldepind
Copy link
Author

From what I can tell from this discussion there is a broad agreement for moving away from the fantasy-land/canonical name so I'd like to move this PR forward 😄

But there has been a few different suggestions about what the new property name should be. I'd like to update the PR to the most popular option. Please react to this comment with your preference and I'll update the PR accordingly afterward.

  • 👍 Use the property name fantasy-land.
  • 🎉 Use the property name instances.
  • ❤️ Use a ES6 Symbol.

@axefrog
Copy link

axefrog commented Nov 17, 2018

Just to throw my hat into the ring, check out:
http://kangax.github.io/compat-table/es6/#test-Symbol

// Using `Symbol.for` to avoid forcing unwanted dependencies on everyone
const value = { [Symbol.for('fantasy-land')]: TypeRef, data: { ... } }

or, to make your own code more concise:

// my-internals.js
export const typeref = Symbol.for('fantasy-land')

// my-app.js
import { canonical } from './my-internals'
const value = { [typeref]: TypeRef, data: { ... } }

It's getting kind of rare to encounter a scenario that doesn't support symbols. In my mind, lack of support for symbols should not preclude their use in a specification, but rather be backed by a suggestion to use some kind of namespace-equivalent polyfill in their absence.

One thing you also lose in not using symbols is that JSON serialization will pick up the extended type reference as a subtree. Probably easily mitigated by simply implementing toJSON(), but still... personally I hate having to think about naming conflicts, or having to deal with them by using long, convoluted namespacing strategies when much better solutions are available.

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.

6 participants