pkamenarsky / replica

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

Support custom wai middleware #10

Closed andrevdm closed 5 years ago

andrevdm commented 5 years ago

Concur-replica PR to support this is at: https://github.com/pkamenarsky/concur-replica/pull/4

andrevdm commented 5 years ago

The PR allows the user to easily add custom WAI middleware. defaultMiddleware provides a reasonable default of supporting static assets from a local static directory. This default is used in concur-replica's runDefault function.

pkamenarsky commented 5 years ago

I'm wondering, is there a strong enough reason to include defaultMiddleware in replica and incur a dependency on wai-middleware-static?

With the new index argument it should be possible to base64 encode and embed resources in the initial index.html directly, or just encode CSS in Haskell and not bother with external style files. The end game being that combined with server side rendering, it should be realistically possible to serve all the app's resources in one go, without additional roundtrips to the server.

Of course there are going to be use cases where serving static files is the way to go. How about a separate replica-static package providing staticMiddleware?

andrevdm commented 5 years ago

I think avoiding dependencies is entirely valid. But in that case I don't think you'd even need a replica-static package. As long as you keep the ability to pass in middleware as this PR supports, then the user can decide which middleware libs to depend on.

So the defaultMiddleware would become something like noMiddleware, remove the dependency and the users still have full flexibility.

I'll update the PR now to work like that and you can see if it feels right?

andrevdm commented 5 years ago

Ok updated, that is much cleaner, thanks

As an example my code that starts replica now looks something like this

  let staticmw = MwS.staticPolicy (MwS.noDots >-> MwS.addBase "static") 
  let mw = myMiddleware . staticmw

  Cr.run 8080 myIndex Wsc.defaultConnectionOptions mw . flip execStateT initState . forever $
    uiStateMachine

myMiddleware :: W.Middleware
myMiddleware app req callback =
   undefined -- .....
pkamenarsky commented 5 years ago

Perfect! One last nitpick - what do we need emptyMiddleware for, since it's just id?

andrevdm commented 5 years ago

Perfect! One last nitpick - what do we need emptyMiddleware for, since it's just id?

Lol, nitpick away, thats how we get good code :P

I thought it read nicer to say Cr.run 8080 myIndex Wsc.defaultConnectionOptions emptyMiddleware than Cr.run 8080 myIndex Wsc.defaultConnectionOptions id

But I was only 50.01% confident about that, so I'm happy to remove it.

Doing so now

andrevdm commented 5 years ago

Updated here and in https://github.com/pkamenarsky/concur-replica/pull/4

pkamenarsky commented 5 years ago

Lol, nitpick away, thats how we get good code :P

Definitely :)

No more nitpicks, thanks a lot for the PR!

andrevdm commented 5 years ago

Thanks :)