ElvishJerricco / servant-router

32 stars 15 forks source link

servant-router and reflex: using the returned `Event t Text` #7

Closed meditans closed 7 years ago

meditans commented 7 years ago

Hi, thanks for this library, it's conceptually very clean. I have only an issue which it's stopping me from using it fully: in your tutorial there is the snippet:

main = routeSite $ \uri -> do
  let handler = books :<|> search
      books i = do
        ...omitted...
        return never
      search Nothing = do
        ...omitted...
        return never
      search (Just keywords) = do
        ...omitted...
        return never

Now, in this example you always return never, but I'd like to actually use the text signal I'm returning to navigate away from the page (it enables me to use more sophisticated navigation).

So, let's say I trigger a text, signalling my intent to move to the page at that url. Unfortunately, for how routeSite is implemented, the address changes in the title bar, but the actual page doesn't change! How could I change the structure in the tutorial so that the firing of the returned Event t Text also change the page I'm in?

Here's the code of routeSite: as you can see, it only sets the url:

routeSite
    :: (forall t m. MonadWidget t m => Text -> m (Event t Text))
    -> IO ()
routeSite siteFunc = runWebGUI $ \webView -> do
    w <- waitUntilJust currentWindow
    path <- getWindowLocation w
    doc <- waitUntilJust $ liftM (fmap castToHTMLDocument) $
             webViewGetDomDocument webView
    body <- waitUntilJust $ getBody doc
    attachWidget body (WebViewSingleton webView) $ do
      changes <- siteFunc (T.pack path)
      setUrl $ T.unpack <$> changes
      return ()

Do you have any suggestion or a more complex example that makes use of the returned event?

qrilka commented 7 years ago

@meditans I suppose https://github.com/reflex-frp/reflex-dom-contrib/issues/19 is a bit more suitable place to address this problem. I suppose routeSite should get fixed. BTW @ElvishJerricco did you have any time to look into it? I'm planning to work on it so it would be interesting to know if something was done already.

qrilka commented 7 years ago

BTW it appears that reflex-dom-contrib already has some code for that, see e.g. https://github.com/reflex-frp/reflex-dom-contrib/pull/21

qrilka commented 7 years ago

And it looks like a solution: it requires some app refactoring and T.unpack as servant-router wants a String as its input

ElvishJerricco commented 7 years ago

Yea, this is an upstream issue with routeSite. And yes, it looks like the pull request that @qrilka linked would solve this.

There's still some changes not on Hackage yet. So I think I will do the following, and then push to Hackage:

meditans commented 7 years ago

Thanks @qrilka and @ElvishJerricco for the answers; regarding the modifications to servant-router, the library will probably need to be modified a bit, as routeSite doesn't exist anymore, and that module has the Route abstraction now. I already did the (String -> Text) changes needed in my fork of servant-router, to be able to use it with the new version of reflex. I could prepare a pr for that, if you want.

ElvishJerricco commented 7 years ago

Whatever you're able to put in a PR would be appreciated.

ElvishJerricco commented 7 years ago

This should be fixed now. @qrilka @meditans, I would appreciate some review on the changes I just pushed before I post a new version to Hackage.

qrilka commented 7 years ago

@ElvishJerricco I have tried to port our app to new servant-router and reflex-dom-contrib but it appears to be not a tiny effort :-\ It looks like the larger question is about noticeable changes reflex-dom-contrib And regarding servant-router I have at the moment 2 minor notices - haddocks don't seem to be updated as there are references to old function names, at the moment I'm not sure that it's doable in a better way but that complete rec section in the example from README looks a bit too complicated for simple library API use. I hope I could get more time to complete the porting so probably I'll come up with some more notes. And thanks @ElvishJerricco

ElvishJerricco commented 7 years ago

Thank you! You're right, I forgot to update the docs. But as for the rec block, unfortunately I can't abstract that into a library function without having servant-router depend on Reflex-DOM, which is undesirable for the server side. Maybe this is ok though, since Reflex-DOM compiles just fine under GHC.