search

reasonml-community / bs-express

Express bindings in Reason
MIT License
210 stars 60 forks source link

"Request" session-support #54

Closed lessp closed 5 years ago

lessp commented 5 years ago

Hi!

Many thanks for creating these bindings, I'm currently working on a project where I needed bindings for cookie-session.

Right now I've built and linked this package locally where I just naively added 

let session: t => option(Js.Dict.t(Js.Json.t));

and 

[@bs.get] [@bs.return null_undefined_to_opt]
external session : t => option(Js.Dict.t(Js.Json.t)) = "";

and put them below signedCookies in Express.re and Express.rei respectively which seems to work.

My bindings for cooke-session is currently very barebones, but could probably publish when I've added more as I go along.

Anyway, would you like me to create a PR to add this, or do you have other ideas?

Thanks, Tom

ncthbrt commented 5 years ago

Hi @lessp. Thanks for this.

I'm a little skeptical about this change, mostly because it session seems to be something which requires an additional middleware to be installed. I know there is precedent with the body field and body-parser, but I still feel kind of uncomfortable about further flipping the dependency hierarchy like this.

Would it not be possible for your session library to take in the request object instead and possibly provide a nicer API to manipulate the session?

lessp commented 5 years ago

Hey @ncthbrt!

Absolutely! It did cross my mind to extend the API for my bindings, but I thought perhaps it’d be more familiar to extend the Request.

Anyway, I’ll do that, thanks! 🙂