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

Namespaced prefixes — next step for resolving name clashes #122

Closed
rpominov opened this issue Jan 20, 2016 · 20 comments
Closed

Namespaced prefixes — next step for resolving name clashes #122

rpominov opened this issue Jan 20, 2016 · 20 comments

Comments

@rpominov
Copy link
Member

So #92 was somewhat resolved with this PR #120

The next step is to eventually publish a version of fantasy-land npm package with namespaced prefixed names.

There are some questions to discuss though:

  • When do we release the new version? Should we wait until majority of libs migrate to the new approach, or should we release it right away so two versions will coexist allowing everyone to choose on which versions to depend in each particular case?
  • How the prefixed names should look like?

Maybe there more questions, these two is what came to my mind.

@rpominov
Copy link
Member Author

I've faced a couple of problems with the new approach. Not sure where better to document them, hopefully this issue is fine.

  1. of is a reserved word, we can't use it for a variable name. Workarounds are:

    - import {map, of} from 'fantasy-land'
    + import {map, of as just} from 'fantasy-land'
    - const {map, of} = require('fantasy-land')
    + const {map, of: just} = require('fantasy-land')
    // Alternatively just import whole object to a variable
    import fl from 'fantasy-land'
    foo[fl.of](1)

    Maybe we could add an alias for of in index.js (e.g. just)

  2. Flow doesn't support dynamic names for methods (neither via class syntax nor via .prototype):

    src/fantasy.js:34
    34:   [concat]( other:Stream<T> ): Stream<T> {
          ^ computed property keys not supported
    
    src/fantasy.js:147
    147: Stream.prototype[concat] = Stream.prototype.concat
         ^^^^^^^^^^^^^^^^^^^^^^^^ assignment of computed property/element. Indexable signature not found in
    147: Stream.prototype[concat] = Stream.prototype.concat
         ^^^^^^ Stream
    

    A workaround I found so far looks like this:

    addFLmethods(Stream.prototype)
    
    function addFLmethods( proto:any ): void {
      proto[concat] = proto.concat
    }

    But this is only a workaround for adding methods to a class, in any case we bypass Flow type checking when will use those class instances. And in general dynamic names can be a problem for any static analysis tools like Flow/TypeScript/etc.

@rpominov
Copy link
Member Author

I was thinking, maybe in the future we could start treating all this as a temporal measure for the transition period and go back to using hardcoded names but with prefixes.

@rpominov
Copy link
Member Author

Another issue: how to use FL compatible libraries in the browser (e.g. in jsfiddle)?
Options I see so far (here foo is a FL compatible lib):

  1. We could make fantasy-land package UMD compatible. Consumers then could load in browser two UMD libs (FL and foo) and they should work fine together (probably, I'm not really sure how dependencies work in UMD).
  2. Bundle fantasy-land as part of the browser build of foo. This is not a great idea, but still could work for things like jsfiddle as long as all libs that are used bundled with the same version of FL.
  3. Fallback to hardcoded unprefixed names in the browser build of foo (basically same as 2. with backward compatibility version of FL).

@SimonRichardson
Copy link
Member

hardcoded names but with prefixes

I'm still not convinced by this, because I don't use ramda or alternatives, so if we do prefix things it just means it's more of a pain for me and an extra barrier for using fantasy-land, the great thing about the spec is that it can be used in both!

Functor.prototype['fl/map'] = function(f) {};
// As an example.
const f = ...
f['fl/map'](x => x);

I wonder if there is a way to have both without sacrificing anything?

of is a reserved word, we can't use it for a variable name

But this is a tooling/linting issue not actual code issue, because running this doesn't cause any issues. So either we create an alias to solve the tooling/linting issue or we just change the name completely. I'm not a fan of just, I'd rather have pure.

Also @rpominov thanks for the feedback, we should consider all of the above going forward.
👍

@michaelficarra
Copy link
Contributor

@rpominov of is not a reserved word. Did some tool fail or did you just make that assumption?

@rpominov
Copy link
Member Author

@SimonRichardson @michaelficarra Yeah, you are right about of. Seems like only Flow has problems with it:

src/fantasy.js:4
  4: import {of} from 'fantasy-land'
             ^^ Unexpected token of

I'll open an issue in Flow repo...

@michaelficarra
Copy link
Contributor

Thanks @rpominov. Please link it here when you do. I'm very interested.

@rpominov
Copy link
Member Author

facebook/flow#1340

@SimonRichardson
Copy link
Member

👍

@samwgoldman
Copy link

One thing, you're right that Flow doesn't currently support computed keys in a number of scenarios, and can't in general, but in the specific cases where the string literal information can be known statically it's possible. We already support string literal types, which enable this feature. I'll take a look to see if we can easily support this use case.

@rpominov
Copy link
Member Author

@samwgoldman That would be great! So yeah computed names are just strings that come from this file index.js I think it could be possible to statically analyse, at least theoretically :)

@Avaq
Copy link
Contributor

Avaq commented Jun 7, 2016

I wonder if there is a way to have both without sacrificing anything? -- @SimonRichardson

In Fluture I did this thing where I create keys for both 'map' and FL.map on my type, so when FL adds namespaces, my API won't change, and FL dispatchers will still work.

@rpominov
Copy link
Member Author

rpominov commented Jun 7, 2016

I think this is the best way to do it, create normal methods in your API like with "old" FL, and then do something like:

Type.prototype[FL.map] = Type.prototype.map
Type.prototype[FL.ap] = Type.prototype.ap
...

We could even provide a helper that does this automatically:

import {makeFantasyLandCompatible} from 'fantasy-land/compat-helper'

makeFantasyLandCompatible(Type)

@dead-claudia
Copy link
Contributor

Just a curious question: why isn't Symbol.for being used? I feel something like exporting Symbol.for("fantasy-land/ap") may work fairly well, since Symbol.for is required to work cross-realm per ES2015+ spec.

@rpominov
Copy link
Member Author

rpominov commented Aug 11, 2016

Symbols may cause some pain in legacy JS environments. Also Flow doesn't currently support Symbols for instance.

On the other hand it's not clear how Symbol.for("fantasy-land/ap") better than just "fantasy-land/ap" for our use-case.

Edit: Although Flow doesn't support dynamic method names either. But anyway Symbols may cause problems and don't give anything in return as it seems.

@Avaq
Copy link
Contributor

Avaq commented Aug 12, 2016

More discussion on the subject of Symbol.for('fantasy-land/x') can be found here

@dead-claudia
Copy link
Contributor

@rpominov I came up with a possible solution to that here.

But I'm still trying to see why namespacing is even necessary at all. Reading the previous discussion on namespacing the methods hasn't really convinced me.

@SimonRichardson
Copy link
Member

This is done.

@rpominov
Copy link
Member Author

@samwgoldman We now have switched to constant prefixed names, which Flow can support but unfortunately doesn't yet.

@rpominov
Copy link
Member Author

rpominov commented Sep 17, 2016

Created an issue in Flow repo facebook/flow#2482

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

6 participants