pkamenarsky / replica

A remote virtual DOM library for Haskell
BSD 3-Clause "New" or "Revised" License
139 stars 14 forks source link

Add header :: HTML argument to app function #6

Closed kamoii closed 5 years ago

kamoii commented 5 years ago

PR for #4. Its working, but I have few concerns.

andrevdm commented 5 years ago

I missed this PR, was looking at the concur-replica repo. I did something similar, well similar goal at least here: https://github.com/pkamenarsky/concur-replica/issues/3

I'm happy to close it if this is a better/simpler approach.

Although I think having a way to add a handler is useful... How else would you host images / fonts etc? So possibly some combination of the two PRs? I'm happy to work together on this if this makes sense

pkamenarsky commented 5 years ago

I like this approach - no need to specify headers in plain HTML at all! In fact, one could replace index.html with something like:

defaultIndex :: ByteString -> ByteString -> HTML -> HTML
defaultIndex indexTitle jsDriver headers =
  [ meta [ charset "utf-8" ]
  , _doctype html
  , html
      [ head ([ title indexTitle ] <> headers)
      , body [ script [ language "javascript" ] jsDriver ]
      ]
  ]

Also, this gets us halfway to server-side initial rendering as well!

@andrevdm I'm not sure, but couldn't you achieve the same by means of a wai Middleware? If not, then we'd definitely need a way to serve resources "out of band" which aren't embedded directly in index.html.

andrevdm commented 5 years ago

@pkamenarsky good point, I'll take a look at the wai Middleware option. That sounds like a better plan. I'll close my other PR for now until I have something working. Thanks :)

kamoii commented 5 years ago

@pkamenarsky Thank you for your reply. I'm glad you liked the approach. I'll continue with this approach. Things todo.

pkamenarsky commented 5 years ago

Wondering how to best approach this. Essentially, we'd be replicating the the DOM types here https://github.com/pkamenarsky/concur-replica/blob/master/src/Concur/Replica/DOM.hs but I don't see how we could extract them into replica since they're framework specific.

Will need to think about this a bit more.

kamoii commented 5 years ago

@pkamenarsky You mean about defaultIndex? I was thinking about directly useing VNode and Attr's constructor.

I realized that we can't express full HTML with current VNode type(e.g. no constructor for doctype, <script>'s contents need a special treatment).Also I came to think it might be inappropiate to ues VNode to express full HTML since VNode purpose is to epxress diffable and patchable dom contents.

Maybe we should leave this idea for now.

pkamenarsky commented 5 years ago

How about https://github.com/pkamenarsky/replica/commit/fd2faf70e51f197cdbc852ee5b0c3172e0e87cb2?

You are right, we need a special VRawText constructor on VNode to make this work, but I guess it could be useful in other contexts too. Other than that, this approach is a lot better than what we have now - implementation improvements aside (like not needing to replace magic strings in index.html anymore), app just takes the whole index.html as HTML now and drops the title argument (which always bothered me):

app (defaultIndex "Title" headers) ...
kamoii commented 5 years ago

@pkamenarsky Looks good :)

pkamenarsky commented 5 years ago

Thanks a lot for the PR @kamoii!