pselm / signals

Purescript implementation of Elm 0.16's signals modules
Other
91 stars 2 forks source link

Solicit feedback on DOM.Renderable #10

Closed rgrempel closed 6 years ago

rgrempel commented 8 years ago

I've been working on a type-class and associated functions for providing a kind of high-level interface to multiple ways of constructing a virtual DOM.

https://github.com/rgrempel/purescript-elm/blob/master/src/DOM/Renderable.purs

I've got it to the point where it does what I need, but I have also tried to design it in a way that could be generally useful, as a kind of rendez-vous for multiple DOM-rendering modules. So, it could be broken out as a separate package.

Before doing that, I thought I would solicit some feedback from some folks that have written some modules that might benefit from this sort of thing (e.g. @bodil @paf31 @AppShipIt @garyb @alexmingoia).

If you have time, could you take a look and see what you think? That is:

Any feedback would be much appreciated!

hdgarrood commented 8 years ago

Looks sensible enough to me. :) I have two suggestions:

rgrempel commented 8 years ago

Thanks! Both those suggestions make a lot of sense to me.

I will need to think of new names for toDynamic and makeDynamic ... perhaps toAnyRenderable and makeAnyRenderable?

hdgarrood commented 8 years ago

๐Ÿ‘

rgrempel commented 8 years ago

I've taken a quick look at some other purescript packages that render to the DOM:

https://github.com/alexmingoia/purescript-pux https://github.com/purescript-contrib/purescript-react https://github.com/slamdata/purescript-halogen

The one where it is seems most likely that DOM.Renderable might be useful is Halogen's use of virtual-dom. (The other two rely on what React does underneath, so they are a bit opaque at the points at which DOM.Renderable could help -- at least, I couldn't spot how to hook into the rendering process at the right point).

Now, I'm going to have to implement Elm's virtual-dom -- in fact, it's next my to-do list, really. So, I may as well do that (taking whatever inspiration from https://github.com/slamdata/purescript-halogen/blob/master/src/Halogen/Internal/VirtualDOM.purs that I can). I'll also need to implement Elm 0.17's version of virtual-dom, which is a little different. Then, I can circle back to DOM.Renderable and see if any changes are needed to accommodate them, and to make it work (theoretically) with Halogen's virtual-dom.

That is, I may as well validate this against a few more targets before I publish it separately.

garyb commented 8 years ago

Pinging @natefaubion on this too, since he'll probably have some thoughts.

natefaubion commented 8 years ago

I don't believe your use of same will do what you think. All typeclass functions take the dictionary as an argument, so when you package up the toAnyRenderable, you are just partially applying render and update with its dictionary, which will give you a new function reference every time, regardless of whether they are constructed from the same dictionary. Maybe if we had constraint kinds you could get the true TC constructor and you could do a referential comparison (terrible dark magick ๐Ÿ˜†). But what you really are looking for is Typeable, which we don't have ๐Ÿ˜ž. I personally would forgo the AnyRenderable stuff because it can't be well-defined with our current machinery, and instead add a Contravariant instance if people really needed a container of multiple types of Renderables.

rgrempel commented 8 years ago

I was wondering about that myself, but it does work -- sometimes the call to same returns true, and sometimes false.

I've just made a little test case, using renderOrUpdate with two different types of Renderable, and it seems to do the right thing -- that is, same sometimes returns true and sometimes returns false (when I watch along in the debugger).

https://github.com/rgrempel/purescript-elm/blob/master/examples/Graphics/UpdateRandomRenderable.purs

And, what old.update and new.update are seeing are the actual functions one might expect them to see -- the dictionaries aren't visible. (Now, one wrinkle is that I've inefficiently doubly-wrapped the AnyRenderable in the example. So, it does always return true at first, since it is seeing the update function for the AnyRenderable itself at first. But then that descends recursively, and gets the right answer (true or false) at the next stage).

My theory is that when I use makeAnyRenderable, I'm unpacking the dictionaries into my RenderableValue type -- essentially, I'm sticking a "scrap your typeclasses" record into the mix, so that I can get the actual functions. That is, I'm taking the instance and unpacking it into a record of functions. So, I end up with the actual functions I want to compare. It really couldn't be otherwise, given the definition of the RenderableValue type.

At least it seems that way -- I suppose the other possibility is that it only works because of a compiler optimization of some kind which unpacks the dictionary. I read up a little bit on referential equality in Haskell, and realized that that is (at least one of) the reasons why you might not want to do this sort of thing -- referential equality imagines a particular kind of computing context, which is essentially dependent on the compiler's implementation (and can either benefit from certain optimizations or be broken by them).

So, I shall take a look at Contravariant and see what can be done with it -- I'm not familiar with that one yet ...

In addition to lists, the use cases are:

natefaubion commented 8 years ago

Hmm, you may be right. Thinking about it a bit more, while it creates fresh dictionary instances for typeclasses, the methods are on the prototype, so they would be sharing the same reference. I think in this case, toAnyRenderable is really the only "safe" constructor for it. While makeAnyRenderable lets you specify the function, it carries with it a referential assumption about the function you give it, while toAnyRenderable only has an assumption around the TC instance, which is more reliable (and I think you could change the check be a constraint kinds hack, which is fairly safe in the absence of Typeable). I guess it depends on if you consider the update a hopeful optimization, or if you want to guarantee anything about its behavior.

Contravariant isn't quite what you want, because a is actually invariant, and you can't hide that existentially because you also require Renderable a on update.

rgrempel commented 8 years ago

It occurred to me that the other way to think this through would be to look at what the compiler produces for makeAnyRenderable and toAnyRenderable. Here they are:

var toAnyRenderable = function (dictRenderable) {
    return makeAnyRenderable(render(dictRenderable))(update(dictRenderable));
};

var makeAnyRenderable = function (render1) {
    return function (update1) {
        return function (value) {
            return AnyRenderable(Data_Exists.mkExists({
                value: value, 
                render: render1, 
                update: update1
            }));
        };
    };
};

So, I can see the dictionaries that you referred to. However, when you refer to constructing a new function via partial application, that doesn't seem to be happening. That is, consider this line:

    return makeAnyRenderable(render(dictRenderable))(update(dictRenderable));

The implementation of the update referred to here is:

function (dict) {
    return dict.update;
}

So, it's not constructing a new function each time ... it's simply extracting the function from the dictionary.

I just saw your last message while typing this ... I'll think about hiding makeAnyRenderable, since it's not essential for my use cases.

And you're right that what I really want is Typeable -- but I think that's a bit complicated to implement? It seems as though it would need some compiler support.

texastoland commented 8 years ago

Maybe if we had constraint kindsโ€ฆ

I still haven't had a chance to read the code (or fully grok constraint kinds) yet but I did see purescript/purescript#2133.

natefaubion commented 8 years ago

Typeable, for what you want at least just requires a typeclass that lets you compare the canonical name of a type, so you can implement a safe coercion. This is actually super easy, but it obviously isn't "safe" unless the compiler does it.

There's some old code defined this https://github.com/joneshf/purescript-typeable/blob/master/src/Data/Typeable.purs

This then lets you create Dynamic, which is just data bundled with its TypeRep.

natefaubion commented 8 years ago

The reason why a constraint kinds hack would be safe is because you could reify the Dict Renderable, and compare the tc.constructor via the FFI. Since we have global uniqueness of instances, this is equivalent to comparing the types. I think the compiler does the single-function optimization to remove the js class, but since you have more than function in your TC it would be fine. It's technically not "safe" now because unique instances could be created with the same implementation function, but this probably does not matter (if they have the same implementation, then it should work anyway).

I might have to take a shower after writing that.

rgrempel commented 8 years ago

I'll give the Typeable approach a try, and see how that feels as an alternative.

I guess that without compiler support, I'd basically be relying on people to supply unique identifiers in their instance declarations -- I suppose I can just make it a "law" that you supply your module name.

rgrempel commented 6 years ago

I've come back to this project after a hiatus, and I'm no longer as fond of this Renderable class as I once was. I may revisit it yet, particularly to "unify" the interfaces between the virtual-dom module and the old Elm 0.16 graphics modules. However, I'm not really convinced any longer that this type-class is useful in general -- I don't think it really captures something sufficiently profound about rendering to the DOM.

So, I'll close this for now, and revisit when I do more work on the graphics modules.