SodiumFRP / sodium-typescript

Typescript/Javascript implementation of Sodium FRP (Functional Reactive Programming) library
124 stars 17 forks source link

Fantasy-land compatability for Cell #30

Closed dakom closed 7 years ago

dakom commented 7 years ago

It seems to me that Cell fits into the category of "Applicative Functor" describe in the fantasy land docs: https://github.com/fantasyland/fantasy-land#applicative

By simply specifying a few prototype methods, this would allow Cell to be used interchangeably by libraries such as Ramda and Sanctuary. This is kindof awesome, since it means we get all sorts of functions for free (e.g. sequence()).

The only lines which must be included, afaik are:

Cell.prototype[FL.map] = Cell.prototype.map;
Cell.prototype[FL.ap] = function (val) {
    return Cell.apply(val, this);
}
Cell[FL.of] = v => new Cell(v);

(this assumes the fantasy-land package is included as a dependency and named FL. The raw strings for keys could be used instead).

It would also be nice to have them defined without the fantasy-land-specific keys as well, so we can simply call Cell.of for example.

I've included this and some tests in my "playground" project in a separate branch, and it currently passes on the practical level (using both Ramda and Sanctuary). See here: https://github.com/dakom/sodium-typescript-playground/blob/fantasy-land/src/tests/functional-libraries/FunctionalLibrariesTest.ts

This leaves a few questions:

  1. Is this okay to add in to the core library? If so, I can make a pull request sometime tomorrow hopefully.

  2. Is there something I missed? I didn't add in any tests for cells as a result of a sequence, or introducing time delays (though the tests are via listen).

  3. What categories does Stream possibly satisfy?

dakom commented 7 years ago

If approved - I'm thinking Cell also satisfies the Monad category via Cell.switchC? I'm not 100% sure how to implement and test that, but if I'm on the right track I'm happy to do the exercise for the sake of learning anyway ;)

graforlock commented 7 years ago

I don't think Sodium has to satisfy Fantasy-land, it wasn't designed to - really. Fantasy land offers some universal language between functional libraries which is cool, on the other hand. But, it is very opinionated and people have many different ideas how to implement, for example, Functors in JS.

dakom commented 7 years ago

I hear that... though as long as it doesn't affect the design of sodium itself in anyway - are there any downsides to adding the above definitions?

To take a specific example, while "people have many different ideas how to implement, for example, Functors in JS.", if in this case it only means setting Cell.prototype['fantasy-land/map'] = Cell.prototype.map, could that cause actual problems when treating Cells as a Functor like any other with fantasy-land libs (i.e. calling R.map() or S.map() with it)?

I guess one could always just monkey-patch from a separate repo, but it would be nice if there was inherent support. Might also improve visibility (e.g. Bacon.js and Most.js are listed on the fantasy-land compliant libs page @ https://github.com/fantasyland/fantasy-land/blob/master/implementations.md)

graforlock commented 7 years ago

No downsides, but it implements them in a different way, for example chain is actually a lift (and in Haskell flatmap / fmap) - that depending on the terminology. Reactive library mostjs has aliases for anything and so implements fantasy land. Is there any merit to it its questionable and I'll let Stephen anwser that question.

dakom commented 7 years ago

Cool. I think a middle ground might be to provide the aliases (e.g. ['fantasy-land/*']) and to include pure law tests in the repo, but not to set the direct-name versions or add tests that depend on Ramda or Sanctuary.

Result would be that one wouldn't be able to directly call something with Cell.of or someCell.ap(), and there won't be any inherent checking that it works with third-party libs, but if one chooses to pass a Cell around to Ramda or Sanctuary it should work out of the box (sidenote - sanctuary needs its own definitions to work in development mode, but I think that's a little too specific to include here)

If approved, I'm happy to work on this and integrate the tests with the proper sanctuary-laws repo, if nothing else as a learning opportunity (though it may not be for a few days or a week or so). Ideally I'll include more tests in my playground demo that would showcase using this feature with ramda/sanctuary.

the-real-blackh commented 7 years ago

I'm generally in favour, but my only concern is that it might add a dependency on a large library. Can you quantify that a little, @dakom? It would be nice if people can use Sodium as a really lightweight library inside the browser. Is it going to add a whole lot of extra code in that situation? If so, I'd be happy to have two versions - one we could call 'sodium-lite' or 'low sodium' or something. :)

Yes, cell is an applicative. Stream can be seen as a monoid with empty value of a 'never' stream and append operation equating to or_else(). If the contained value is also a monoid you could use merge, but unfortunately this is clunky and makes the more general definition with or_else impossible to have at the same time. Also, we could make a version of merge that treats the contained value as a monoid. Cell is, as you observed, also a monad with switchC being the join operation, but how useful that is for practical purposes I've never been quite sure.

dakom commented 7 years ago

OK great! We can avoid the dependencies altogether for production build by just using the fantasy-land string literals (that's the approach Sanctuary uses). The tests will add some dev dependencies, though they are pretty lightweight I think.

Thanks for the clarification of where things fit in. I guess I'll try to do the PR for Cell sometime within the next week or so (adding the ~4 lines above is no biggie, but I need to figure out how to integrate the testing framework properly and it seems like it's currently waiting for some merges, e.g. this and this).

I'm not entirely clear on the Stream as a Monoid and specifically what the implications of this are:

we could make a version of merge that treats the contained value as a monoid

But it sounds like good stuff to me :D Wish I did understand it better... but I'm still sortof at the stage of struggling with reading type signatures :(

graforlock commented 7 years ago

I don't see any merit in making it Fantasy land standard compliant.

the-real-blackh commented 7 years ago

@graforlock I think standard functional type classes are important. I don't know the background to it in Typescript, though.

@dakom So, a monoid has an 'empty' value and an 'append' operation <>, which is not commutative (that is, order matters: a <> b != b <> a). 'never' and 'orElse()' will satisfy these.

merge() won't satisfy these requirements because it requires an append operation as its second argument. Now, if we assume that the value a is a Monoid, then we can use the monoid append operation automatically. BUT - then Stream<A> is not a monoid in general. Only Stream<A> where A is a Monoid is a monoid. In Haskell this would be written as instance Monoid a => Monoid (Stream a) where. The first one is more general because Stream<A> is a monoid for any A. The second would be more useful in practical situations because you almost always want merge() rather than orElse().

dakom commented 7 years ago

Thanks again for the explanation! I read up a bit more (this page was great), but must admit I'm still not following why it would not work to define orElse as the Stream Monoid's append. For Streams a,b,c it satisfies the test of a.orElse(b).orElse(c) === a.orElse(b.orElse(c)) ... is it because the decision to merge left in the case of simultaneous events is (in a math sense) arbitrary?

Happy to learn more!

the-real-blackh commented 7 years ago

@dakom, It would work be 100% valid to use orElse() as the monoid's append. I'm just making the point that a lot of the time, merge() is what we would prefer, and using orElse() completely prevents that.

dakom commented 7 years ago

OK cool - could we include a note about this in the README, something like the following under a general note of fantasy-land compatibility? (if so I'll draft it in with the PR)

"Stream is fulfills the Monoid spec by using orElse() as an implementation of append (<>). If you prefer a different strategy, you can monkeypatch Stream.prototype['fantasy-land/concat'] to use merge() like so..."

the-real-blackh commented 7 years ago

Sure, sounds good. I don't know if it's possible in Fantasyland, but is it possible to define it as merge() and have it just fail if the A type is not a monoid? I just think that will give better code generally.

dakom commented 7 years ago

I apologize, I need a little more help understanding.

Let's assume we define concat as something like this (other is always guaranteed to be a Stream of the same type):

Stream.prototype.concat = function(other) {
    return this.orElse(other)
}

Are you suggesting that if other's events are also a Stream (or anything with a concat really), then it would be something like:

Stream.prototype.concat = function(other) {
    return this.merge(other, (left, right) => {
    //What goes here? Something based on other's events?
    });
}
the-real-blackh commented 7 years ago

I'm suggesting that the other value is also a monoid, then you would use that as the merge argument, e.g.

Stream.prototype.concat = function(other) {
        return this.merge(other, concat);  // ...where the right 'concat' is derived for the type of A, given stream<A>
    };
dakom commented 7 years ago

Oh I understand... will look into it, I'm not sure if typescript's generics support this out of the box since type information is erased from the resulting js, but I'll give it a shot!

dakom commented 7 years ago

It seems that the only way to check for properties on A's prototype is if an instance of it (or rather its class constructor) is passed as an argument. Since there's no guarantee that there's something to check, e.g. .firings[], I think we'd have to modify Stream's constructor to accept this as a new parameter. This is relatively idiomatic in Typescript, and imho it's not that big of a deal for the end-user to pass it with StreamSink etc. - but what do you think?

Reference: "Using Class Types in Generics" - https://www.typescriptlang.org/docs/handbook/generics.html

Assuming we've solved that problem, for the sake of example, let's imagine we know that A is Stream. I'm still not quite sure how that should expand:

function concat(me:Stream<Stream<string>>, other:Stream<Stream<string>>) {
   return me.merge(other, (l,r) => concat(?));
}
dakom commented 7 years ago

Latest PR adds Functor, Apply, and Applicative aliases for Cell, with the official tests. Monad is also supported but currently untested.

For right now it's linking to the queued branch on fantasy-laws to import the tests, but I'll keep my eye on it and switch it to master or npm once they've merged.

I also added a couple practical tests with Sanctuary (for S.lift(), S.sequence(), S.join(), and S.chain()). note: the extra libs did not add any weight to production build, only for testing. The actual aliases are just a few lines of straight code.

EDIT: as mentioned above, currently the tests are not passing to check Cell as a Monad, though I think it's due to the wrong test setup rather than the wrong alias definition (since the practical tests of S.join/S.chain do pass). Will look into it and hopefully fix - for the current PR I just uncommented that part so it could be merged safely.

the-real-blackh commented 7 years ago

'chain' is called 'bind' or >>= in Haskell. Cell.switchC is exactly the monadic 'join'.

bind can be defined in terms of join or vice versa. Here's it is in Haskell:

(>>=) :: Monad m => m a -> (a -> m b) -> m b
ma >>= f = join (fmap f ma)

join :: Monad m => m (m a) -> m a
join m = (m >>= id)

So, what you wrote is correct:

'fantasy-land/chain'<B>(f: ((a:A) => Cell<B>)):Cell<B> {
      return Cell.switchC(this.map(f));
}
dakom commented 7 years ago

Heading out now, won't be able to followup till tomm, but I believe the latest pull request closes this issue as far as Cell goes.

(note - I also noticed that Cell fulfills Comonad/Extend via sample() so I added that in)

If that all checks out, maybe we can open a new issue to pick up the discussion on Stream fresh? It's technically working I think (via orElse for now) - I added in a "practical test" using S.concat, but the unit tests aren't in yet (need to figure out how to do async with jsverify)