garyb / purescript-virtual-dom

Low level virtual-dom bindings for PureScript
MIT License
30 stars 7 forks source link

Update to virtual-dom 1.0.0. #6

Closed fluffynukeit closed 9 years ago

fluffynukeit commented 9 years ago

Removed vtree dependency because it now is part of virtual-dom. Also, added widget, thunk, vhook, and tests.

I brought the bindings a little bit more up-to-date by adding vhook, thunk, and widget. Widget, I think, is especially important for integrating with foreign javascript GUI components, such as graphing libraries. I'm hoping to combine vdom (with eventual blaze combinators #2), signal, and some plotting libraries to make a data viz app.

garyb commented 9 years ago

Nice, thanks! I hadn't noticed the 1.0 release.

Could we perhaps avoid using open records for the widget, hook, etc. bindings and instead make them require a value for each "method" as a Maybe'd function instead? Either as just normal arguments or via record again.

fluffynukeit commented 9 years ago

We're definitely thinking along the same lines. The current open records strategy leaves a bad taste in everyone's mouth, I think. purescript-virtual-dom-typed tried out an alternative strategy using a list of Attributes to fix that, but I don't feel like it's the right way to go.

I'm planning on trying out typifying all of the vtree and friends methods that accept objects of properties using purescript-foreign-options. (I have used it pretty successfully for handling the large amount of options, both required and optional, in purescript-dygraphs.) For this first PR, I just wanted to add the widget, hook, and vhook functionality in a way that seemed consistent with the style already in use for vnode so that we'd still have functionality for those in case there are issues with my purescript-foreign-options experiment.

Would you prefer that this forthcoming typifying experiment be added to this PR, or should I go ahead and do it on a new PR as I originally planned?

fluffynukeit commented 9 years ago

I've discovered there's something wrong with the vhook implementation in the PR or something goofed up in the vhook test. Will add fix to the PR when ready.

garyb commented 9 years ago

Unless things have changed recently I think the hook and unhook functions have to be attached to an object's prototype rather than the object directly - there's a hasOwnProperty check to determine where hook comes from in is-vhook.

fluffynukeit commented 9 years ago

Yes, that was the issue. I rebased an update for vhook. I'm fairly unskilled with javascript, mostly stumbling through by reading articles about e.g. prototype, so if something is weird with my implementation, please let me know.

As an aside, I think this is a case in which it would help to have something like FnEff0, 1, 2, 3, etc. in the prelude with the other FnX types. Is there such a type in prelude or in any other library? The hook and unhook callbacks would then both be of type FnEff2 Node String Unit.

garyb commented 9 years ago

That will probably work, but I'd perhaps suggest something more like this:

var vhook  = (function() { 
  return function (props) {
    var rVHook = function () {};
    if (props.hook)   { rVHook.prototype.hook   = props.hook };
    if (props.unhook) { rVHook.prototype.unhook = props.unhook }; 
    return new rVHook();
  };
}());
garyb commented 9 years ago

Thanks. Are you happy for this to be merged now?

fluffynukeit commented 9 years ago

Yes, sorry for not mentioning it! I got caught up debugging the typification experiment. I think this PR is ready to go.

garyb commented 9 years ago

Oops, forgot I hadn't merged this one. Thanks for your efforts!