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

Thoughts on improvements #35

Closed
ondrap opened this issue Nov 17, 2015 · 20 comments
Closed

Thoughts on improvements #35

ondrap opened this issue Nov 17, 2015 · 20 comments

Comments

@ondrap
Copy link

ondrap commented Nov 17, 2015

I started developing an application with reflex-dom about a week ago, so maybe some of my thoughts might be wrong. My experience is so far quite good, but here are some thoughts...

  • I installed ghcjs 7.10.2 (not the try-reflex) and I had to use https://github.com/k0001/reflex-dom/tree/develop-ghcjs-0.2.0 . Any plans on integrating it into your repo?
  • I am using servant with structures from persistent, which compiles (at least for the datatypes) directly into ghcjs. It works wonderfully (I absolutely recommend it), however my server structures are in Data.Text. Ghcjs seems to work correctly with Data.Text (probably bettern than with String). Are there any thoughts about converting the whole reflex-dom to Text? It seems to me there actually is no reason to use String at all, especially with the OverloadedStrings extension. The thing is that just changing types in the reflex-dom files from String to Data.Text.Text probably does the trick.
  • I have a little problem with large page loading lots of elements where many of them are hidden. I'm not sure how it works internally, but it seems to me that the elements are first put to page and only in the next step the Dynamic variables are applied (e.g. for the class=hidden attribute). Is there a simple way to evade the problem?
  • I still have a non-composable feeling. Like: I suddenly need some div to be shown or hidden on some slightly more complex condition and I just have to wire the code with mapDyns, 'combineDyn's, <=< (I finally found use for this operator) - while I just wanted to say hide if a ==0 && b && !c.

E.g. I had to change the button to buttonClass, but had to change the signature to accomodate the simple fact, that I needed to put a glyphicon into the label text.

buttonClass :: MonadWidget t m => String -> m () -> m (Event t ())

I am developing with the CSS bootstrap framework that relies quite a lot on class attribute. What might cleanup the code may be making a Monoid (e.g. newtype ElClass) for the class parameter.

Anyway, so for it's not a bad experience, it just works :)

@ryantrinkle
Copy link
Member

Hi @ondrap; these are really great suggestions.

  • Yes, we will be updating to the latest ghcjs, ghcjs-dom, etc., pretty soon. However, reflex-dom pushes ghcjs pretty hard, so we need to do a lot of testing first. I'm glad that it has "just worked" with you - that's another datapoint showing that things are looking stable, which will help us get it done sooner.
  • It might make sense to switch to Text. I did experimentally switch to JSString a few months ago, but, surprisingly, it had no impact on performance, so I deprioritized working on the strings situation.
  • Without knowing the details of your situation, my initial recommendation would be to see if you can ensure that the initial value of the Dynamics includes the "hidden" class. If your use case prevents that for some reason, I'd be interested in taking a look and seeing if I can support it better.
  • Making operations like mapDyn and combineDyn pure (i.e. not monadic) is one of my top priorities for Reflex. There are some things that make it difficult, but it will be coming fairly soon. You will still need to treat Dynamics as (Applicative) Functors, but it should be a pretty big improvement.

Thanks for taking the time to try reflex out and provide such detailed feedback. This sort of thing is very helpful for figuring out the future direction of the libraries and making sure that it does what people need.

@ondrap
Copy link
Author

ondrap commented Nov 18, 2015

Thanks for the response, these are great news.

Regarding the 'hidden' class - it seems to behave the same way in both Chrome and Firefox. I think I really do have the Dynamics set to hidden initially. I have one big page (in production I'll probably trim it) with lots of records (using simpleList) - with quite a lot of hidden stuff. I may try to make a contrived test case, but in the meantime it's roughly this.

Could it be that my 'initial dynamic' is actually derived over dynMap from other dynamics? Like the one bellow?

dbRuleWidget = do
  rulesLoaded <- runApiCheck apiDbRuleList postBuild -- Loads data from databse
  rules <- holdDyn [] rulesLoaded
  simpleList rules $ \rule -> do
    dbRulePanel rule

dbRulePanel entrule =
  elClass "div" "panel panel-primary" $ do
    editbtn <- glyphButton "edit" "Edit"
    editDyn <- holdDyn False (const True <$> editbtn)
    showIf (== True) editable $ do
        buttonClass "btn btn-primary" $ text "Save"
        buttonClass "btn btn-default" $ text "Cancel"

showIf :: MonadWidget t m => (a -> Bool) -> Dynamic t a -> m b -> m b
showIf f dynvar code = do
  let mkAttr True = ("class" =: "show")
      mkAttr False = ("class" =: "hidden")
  attr <- forDyn dynvar $ mkAttr . f
  elDynAttr "div" attr code

@ondrap
Copy link
Author

ondrap commented Nov 18, 2015

Sorry to bother you with other thoughts... but I'm just in the process of creating some in-place editable form and it occurred to me that the requests for templating do make a lot of sense - but from a completely different view.

We like to create components that have perfectly defined input and output. However, within the component, we do not generally want this separation. In particular, what I'm doing right now is:

namedyn <- elClass "div" "panel-head" $ someInPlaceEditable ...
(priodyn, enableddyn, packagedyn) <- elClass "div" "panel"body" $
    (priodyn, enabledyn) <- elClass "..."
    packagedyn <- elClass "..."
    return (priodyn, enableddyn, packagedyn)

That really seems to me quite hairy. The following is just a sketch of an idea - it is possible to rip an element out of DOM and put it somewhere else (or better yet, just create it and insert later). So, today, one could probably write something like:

(nameddynE, nameddyn) <- input' ...
(priodynE, priodyn) <- input.a.
...
elClass "div" "panel-head" $ implant nameddynE
elClass "div" "panel-body" $ implant priodynE
...
implant n = do
   parent <- askParent
   performEvent_ $ const (appendChild parent (Just _el_element n)) <$> getPostBuild

It wouldn't work if it was implanted more than once, but it seems to me this would lead to very easy templating and would greatly reduce the 'pushing-up' of the events/variables.

@mightybyte
Copy link
Member

@ondrap FYI, we have an editInPlace widget in reflex-dom-contrib here.

Your hide point is interesting. I think the reason it's not as composable as you would like is multi-faceted. First, the point Ryan mentioned of making mapDyn pure would certainly help. But on top of that, there are quite a few different ways to do it. Here are a few of them off the top of my head:

  1. A bootstrap-specific way by setting the show/hidden classes.
  2. More generically with style=display:none or style=visibility:hidden
  3. Even more generally by wrapping with another widget that takes the elements out of the DOM completely when they are not visible.

So I think we're suffering from the paradox of choice a bit here. It's not that it can't be done, but rather that nobody has decided to be opinionated enough to choose one option.

@ondrap
Copy link
Author

ondrap commented Nov 18, 2015

I tried to refactor my 'slightly complex' page using:

implant :: MonadWidget t m => El t -> m ()
implant n = do
   parent <- askParent
   void $ liftIO $ appendChild parent (Just $ _el_element n)
rip :: MonadWidget t m => m a -> m (El t, a)
rip = el' "span"

(the rip function to help with objects that do not return the El t) and my feeling is that the code got much cleaner. I didn't intentionally head this way, byt it really start to seem like CMV framework. The code now looks like:

(cancelEditN, cancelEdit) <- buttonClass' "btn btn-default" $ text "Cancel"
-- +logic around other buttons etc.
-- ...
divClass "panel-body" $ elClass "div" "pull-right" $ implant cancelEditN

I just like it much better this way. It would be even better, if I could abstract the 'Event' from the button in the first part and 'attach' it to the button later with implant. Considering that the event is just an event handler, it should be possible to create a pair (event handler, event) in the first step and then later attach the event handler in the 'template'. What do you think?

@ondrap
Copy link
Author

ondrap commented Nov 19, 2015

The flickering problem: the problem is that Dynamic attributes are set in the postBuild phase. If there is a elDynAttr, it is initialized without any attributes. I made myself a helper function, that works mostly as expected - with the provision, that instance Attributes Dynamic... doesn't do difference lists on initial value of the dynamic - an attribute set with initial attribute doesn't get removed if it is not present in the dynattr.

elInitDynAttr' :: MonadWidget t m => String -> Map.Map String String -> Dynamic t (Map.Map String String) -> m a -> m (El t, a)
elInitDynAttr' n attr dynattr code = do
  (e, res) <- elAttr' n attr code
  addAttributes dynattr (_el_element e)
  return (e, res)

@ryantrinkle
Copy link
Member

Yes, I think you're right about the attributes. I'll see what I can do about this.

I definitely see what you're saying about how clean the code looks. The issue of decoupling the logic structure from the layout structure is one I've thought about a lot, and I would love to have some more tools for doing that. However, for me, the cost of using something like implant would be too high.

In the limited use case you mentioned, in a single function scope and with a single developer writing code over a limited span of time, I think it would probably work out alright. However, with more developers working on the code, losing the once-and-only-once placement guarantee would become a much more serious problem. For example, as a widget becomes more complex, with helper functions, etc., it will become more and more difficult for a developer to determine whether and where an element declared in the logic is actually placed into the DOM—and therefore easier and easier to delete the use of an element or create an extra use of it. Basically, this means that developers need to always diligently track things down when changing the code. Nobody can be diligent 100% of the time, when deadlines, distractions, and life get in the way.

Furthermore, it's not just that this requires developers to diligently hunt things down whenever they make a change—it opens the door to situations where it's not even possible to statically determine whether an element is placed. For example:

w = do
  (theElement, _) <- buildTheElement'sLogic
  x <- getSomeInput
  divClass "a" $ do
    when (x < 5) $ implant theElement
  divClass "b" $ do
    when (x > 5) $ implant theElement

Whoops! Now the widget works for x /= 5, but fails—during testing, if you're lucky—when x == 5. Of course, in a real application, the conditions are likely to be far, far more complex, and the failure modes far more subtle. Imagine, for example, a situation where elements are placed into a datastructure of some kind, passed around the application (or even a complex widget), then placed later. This is something I would not be surprised to see in the implementation, for example, of a sortable table widget. I would be surprised, however, if it were bug-free.

There is a "Right Way" to do this: linear types. With linear types, we could specify, at the type level, that each element must be placed exactly once.

However, linear types aren't available in Haskell (yet?), so we have a difficult trade-off to manage. One way we might be able to have a solution that preserves the placed-once constraint while also allowing more elegant DOM layout would be to use Template Haskell to turn a layout into an expression that draws it and a pattern that binds all of the elements inside. Ideally, I'd like the result to look something like this:

myWidget = do
  rec [layout|
        some layout that places a button and names it x
          and displays the dynamic value y
      |]
      y <- count $ domEvent Click x

Unfortunately, I'm not sure how this could be achieved with the current Template Haskell, because it doesn't have a way of splicing in a bind statement (e.g. do { x <- asdf }). If we could splice in a bind, the above would desugar to something like:

myWidget = do
  rec x <- do
        var1 <- button "Click me"
        display y
        return var1
      y <- count $ domEvent Click x

I'm not completely sure what it will take to make this happen, but I'd love to hear your (and others') ideas.

@ondrap
Copy link
Author

ondrap commented Nov 22, 2015

What about leveraging HList? Something along the lines:

{-# LANGUAGE QuasiQuotes #-}
{-# LANGUAGE ViewPatterns #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE KindSignatures #-}

import Data.HList

let okbutton = Label :: Label "okbutton"
    nameinp = Label :: Label "nameinp"
    [pun | okbutton nameinp|] <- runWriterT $ do
              el "div" $ el "something" $ tell (okbutton .=. button "ok")
              el "div" $ el "something" $ tell (nameinp .=. input def)

@ryantrinkle
Copy link
Member

That could be an interesting approach. How would over- and under-placement be dealt with?

@ondrap
Copy link
Author

ondrap commented Nov 22, 2015

I just thought about it a littlemore and the runWriterT won't work - the type of the Monoid is changing. However using the HList may still help, I'll try to explore this a little.
What do you mean by over- and under-placement?

@ali-abrar
Copy link
Member

I think he means ensuring that each element is placed into the DOM exactly once.

@oliver-batchelor
Copy link
Collaborator

Hi guys

Just to weigh in with my two cents,

I am slightly surprised no one has mentioned widgetHold - it is designed
for the purpose of switching in and out parts of the DOM - it doesn't serve
quite the same purpose as creating pre-made templates or hiding large
sections but if Ondrej has been using reflex for a week it is quite
possible that he just hasn't seen it?

Regarding flickering with attributes, I think ultimately the solution has
to be transactional (i.e. animationFrame) based rendering, but that's no
small change!

Oliver

On Mon, Nov 23, 2015 at 12:14 PM, Ali Abrar notifications@github.com
wrote:

I think he means ensuring that each element is placed into the DOM exactly
once.


Reply to this email directly or view it on GitHub
#35 (comment)
.

@emmanueltouzery
Copy link

regarding the implant idea and the idea of inserting the rendering someplace else... I've had the same issue in my app and made a pretty primitive dynAtEltId -- very unsafe, it relies on the element ID to overwrite with your render...
https://github.com/emmanueltouzery/cigale-timesheet/blob/master/src/WebGui/Common.hs#L232-L259

The reason I did that was because of modals. I have a single modal div at the toplevel of my DOM, and I use that function to overwrite the modal content before displaying the modal. Before I had that, I had two options: modal div at the toplevel, complex widgetHold logic to overwrite the modal contents, and bringing up the events all the way up from my GUI to the modal widgetHold at the toplevel... Or, option 2, actually have my modal divs all the way down where they get triggered, which caused other issues: almost always the modal was meant to edit the data displayed in the main view. So when the user closed the modal, I would refresh the main view. But due to timing, I would refresh the main view when the user clicked the button, before the modal close animation would conclude, and wiping the part of the DOM with the modal (since I brought it all the way down), before the modal finished hiding looked very bad. So I had to trigger the action not when the button was clicked, but when the modal hiding animation finished, which looked also pretty bad (though less so than the first option).
Now I've settled on that dynAtEltId. It's less type-safe than the proposed implant, but I don't need to pass details down to the functions too much. I just have the modal ID as global constants for the whole app. I could leverage a monad reader to avoid that though. Anyway, looking forward to a all-around better solution, but I think something like that is needed.

@ondrap
Copy link
Author

ondrap commented Jun 4, 2016

After some playing with forms, I tried a different approach that seems to work nicely for forms. It is slightly incompatibile (when some non-trivial default is used) with 'dyn/widgetHold', I will probably do some pull request when I get time.

I have added instance of StateT FormState to MonadWidget (I didn't quite test it yet with the widgetHold/dyn, these were quite tricky, but I think it should work). In the FormState I have a Dynamic formresult and Dynamic Bool (to allow validation). The widgets (input forms etc.) get lens and validation function as a parameter. As a result I can make a form like this:

content pkg@(Package name desc version depends) =
   GF.form $ do
     runForms pkg $ do
        GF.fGroup $ formInput name (Just "Name") "Name" "text" checkEmptyField packageName
        GF.fGroup $ formInput desc (Just "Description") "Description" "text" Right packageDescription
        GF.fGroup $ formInput version (Just "Version") "Version" "text" Right packageVersion

this results in Dynamic (Maybe Package). In order to get the StateT have the MonadWidget instance I had to implement MonadSample, MonadHold and HasPostGui instances; these classes already had a StateT instance, but for State.Strict. Was there any reason to have it just for the strict state?

@qrilka
Copy link
Contributor

qrilka commented Jun 24, 2016

Hi @ryantrinkle it's already more than a half a year since you were talking about pure mapDyn and combineDyn - could we know if there was any progress with it?

@ryantrinkle
Copy link
Member

@qrilka Sure! It's in testing. Check out the functor-dynamic branch of reflex, and the refactor branch of reflex-dom. Once I'm confident that everything is good to go, both will be merged to develop and released to hackage.

@mightybyte
Copy link
Member

@ryantrinkle I was looking at this last night. Could you put together a branch on reflex-platform that integrates these? I tried, but it wasn't obvious to me everything that I needed to do.

@ryantrinkle
Copy link
Member

Yes, it's the new-reflex-dom branch.
On Jun 24, 2016 11:44 AM, "Doug Beardsley" notifications@github.com wrote:

@ryantrinkle https://github.com/ryantrinkle I was looking at this last
night. Could you put together a branch on reflex-platform that integrates
these? I tried, but it wasn't obvious to me everything that I needed to do.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#35 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABGlYPsu6_GgU6yW_7Ce9QYrpqyWF3wrks5qO_tbgaJpZM4GkS5U
.

@mightybyte
Copy link
Member

Perfect!

@ryantrinkle
Copy link
Member

@ondrap I think we've gotten everything mentioned here integrated into develop at this point; if there's anything you're still looking for, please let me know! Thanks for the in-depth analysis.

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

7 participants