rgrinberg / opium

Sinatra like web toolkit for OCaml
MIT License
755 stars 67 forks source link

Notes as I build a site with Opium #26

Open avsm opened 9 years ago

avsm commented 9 years ago

I hope you don't mind if I maintain a little issue list that I'm running across as I make my Opium site. I'll submit pull requests for these right after the site is done and if you think they are good ideas. Will edit this as I go along.

rgrinberg commented 9 years ago

The first 3 are good points and number 3 is something that I've looked at especially before.

I'll address the last point a little more thoroughly

avsm commented 9 years ago

Thanks, makes sense. So Opium can be thought of as a 'Rock executor' since it takes a set of Rock middleware and links it through to a command-line interface and other such niceties. So a well-designed modular use of Opium is to have as much logic as possible be pure Rock, and then just glue it together into an Opium app. (It's working out very well so far btw, will open the source just as soon as it's all tied together).

avsm commented 9 years ago

New entry: cant get cookies working. I added App.middleware Cookie.m but Cookie.get in a subsequent middleware fails. Can middlewares depend on previous ones setting the environment like this? Cookies work fine via manually adding them to the Rock.Request.headers using Cohttp's header generation directly.

rgrinberg commented 9 years ago

Hmm I just tested it and setting cookies as in examples/sample.ml works fine. Can I see how you're doing it?

You may set the cookies directly into the cohttp header if you want. The only reason why I have the wrapper middleware is because I want the values to be base64'd by default to avoid having to remember to escape them myself.

rgrinberg commented 9 years ago

Addressing your second point https://github.com/rgrinberg/opium/blob/master/opium/app.mli#L88

Isn't that good enough?

rgrinberg commented 9 years ago

I'll make a ticket for the Lwt_log thing. Setting debug should definitely affect it but it should also be its own command line flag.

avsm commented 9 years ago

@rgrinberg I'll have to extricate the cookie problem into a smaller test case (my current code is annoyingly part of a big blob and hard to pull out as-is). Will do so as soon as current hacking settles down.

Re: the redirect, there seem to be a bunch of convenience functions defined in Opium that aren't in Rock. I do the redirect in a handler, so I really need a Rock.redirect rather than an Opium.redirect so that middleware can do the redirection (in this case, to go to a login page)

rgrinberg commented 9 years ago

Yes, some of these helpers are actually general enough to just be in rock. I will move them over and re-export in opium.

anuragsoni commented 5 years ago

@avsm Not sure if you are still using Opium. From the points that are left unanswered is there anything else that I can improve in Opium? :)