garyb / purescript-virtual-dom

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

Use foreign opts #7

Open fluffynukeit opened 9 years ago

fluffynukeit commented 9 years ago

This PR includes two changes beyond PR #6.

The first is a leaner, more convenient definition of thunk that automatically handles converting null vtree with Nothing, or non-null to Just.

The larger and more philosophical change is adding increased type safety with new dependency purescript-foreign-options. The library handles conversion of Maybe, Either, callback functions, etc., from PS conventions to Javascript conventions. The configuration options for vhook, widget, and vnode must now contain prerequisite fields and types in order to compile.

The updates to thunk are largely orthogonal from the options changes, so if you only want one or the other, just let me know and I'll update either this PR or #6 as requested to make the merge easier.

garyb commented 9 years ago

I like it. I'll have a bit of a play before merging though, and see if anyone else has any comments too. (@paf31?)

paf31 commented 9 years ago

I've not really been keeping up to date with this library, but I'm familiar with the options library, and the approach makes sense given the problem.

There is also the purescript-options library which takes a different approach to options objects, so maybe that's worth a look too.

fluffynukeit commented 9 years ago

@garyb, we had some discussion of these approaches recently at purescript/purescript-foreign#20. The main change to purescript-foreign-options since then is the addition of the Opaque constructor to protect parts of the options record from conversion.

garyb commented 9 years ago

Thanks, I was sort-of paying attention to that discussion but had intended to look it up again for this. :)

garyb commented 9 years ago

I definitely appreciate the effort that went into this, and agree something like this is the right direction, but I think I'd rather keep it dumb and accepting anything in the props objects for now.

This library was intended to be a low-level thing that proper interfaces were built on top of, and since there's not a clear winner in the foreign-options department yet (I also have some ideas I'd like to try out) and it's not entirely overhead-free I think it might be worth holding off until we come up with something even better.

fluffynukeit commented 9 years ago

OK, that's reasonable. If you try out something of your own, could you write a note in this PR about it? I'd enjoy taking a peek at any new approaches. Thanks.

fluffynukeit commented 9 years ago

Would you consider taking the changes to thunk only? They are unrelated to the use of the options library, and they are more consistent with how vdom actually works internally. I can prepare a new branch including only that change to make it easier if it's something you're interested in.

garyb commented 9 years ago

That'd be great, thanks.