polettix / Dancer-Middleware-Rebase

A Plack middleware to be used for Dancer
1 stars 0 forks source link

Better package name? #1

Open nicolasfranck opened 9 years ago

nicolasfranck commented 9 years ago

The package name suggests that Dancer is needed to make this module work. But it's simply a Plack middleware, that can work with any plack based framework.

Would "Plack::Middleware::Rebase" not be a better name?

polettix commented 8 years ago

Hi!

Sorry for the late reply, but I didn't see the notification in the first place and was not aware of this issue at all. My fault :(

I'm not sure about the general applicability of this module - only tested internally for a few Dancer applications actually. Do you reckon it would be more general actually? I'm not even sure it's useful for Dancer 2.

nicolasfranck commented 8 years ago

I don't know anything about Dancer2, but I think it's usefull for plack applications that run behind a proxy, and that do not get any information from the frontend server how to be reached (there is no common way). The only flaw I see, is that only request->uri_for and request->uri_base are fixed by this package, while request->port and request->host remain unaltered.

Maybe confer with the author of Plack::Middleware::ReverseProxy? That module derives host and port from headers, sent from the frontend-server, but it fails in deriving the path prefix under which the dancer application is served, because there no way send this.

polettix commented 8 years ago

... so there are two modules that do two different halves of what's actually needed...

I'll try to look into P::M::RP and see if I can push some change so that it can evolve to have it all. Living in the Plack namespace it's probably the better candidate to remain alive.

polettix commented 8 years ago

I'm inclined to say that using both Plack::Middleware::ReverseProxy and Plack::Middleware::ReverseProxyPath should get the job done. I'll try to ping Miyagawa anyway.

polettix commented 8 years ago

After thinking a bit about it, there's actually a difference in that this module allows you to put the configuration close to the application, while the other close to the reverse proxy. The latter is probably the sane thing to do, but there might still be value.

nicolasfranck commented 8 years ago

True. As I said, there is no common way in the http protocol to deal with this situations. Either you configure the path in the reverse proxy, or you do it in the backend server (but not both, so it's not two halves like you said).

My point was that your package is Plack::Middleware (and it does not anything more), while the name suggests it's Dancer::Middleware, which it isn't. So it's more broadly usable. Plack based frameworks like Dancer and Mojolicious start from the same base, and should make no more assumptions, so why wouldn't it work for all of them?

Anyway, people already use it outside of Dancer ;-)

polettix commented 8 years ago

I was actually considering the rename as you suggested, after my previous consideration (i.e. the use case for Rebase being slightly different from ReverseProxyPath, or at least providing different knobs in a different position). I'm just trying to consider whether it's better to keep the code as-is (which is somehow restricted to my personal needs in the past) or expanding to something more general, e.g. copying ReverseProxyPath for the "write" part, while taking the input parameters from the configuration instead of from other headers for the "read" part.

The point is that I tried this with Dancer only and somehow I crafted the headers according to what I know that worked for Dancer. Is this crafting general enough?

polettix commented 8 years ago

FYI, I decided to play with the idea of a generic $env mangler and I created Plack::Middleware::MangleEnv.

This should get you covered for most of the functionalities provided by this module, except the strip option (which I tend to consider non-essential anyway, because it's easy to not set the part to strip in the reverse proxy).

The equivalent Dancer configuration for the example in the README.md would then be:

plack_middlewares:
  -
    - "MangleEnv"
    - manglers
    - 
      psgi.url_scheme: http
      HTTP_HOST: example.com
      SCRIPT_NAME: /app

The aim of the new module is to also allow you to provide configurations via environment variables. I'm heavily playing with Docker these days, and this would allow me to just set a couple of environment variables in a centralized location, without the need to hunt a lot of configuration files. Hence, you can do this:

plack_middlewares:
  -
    - "MangleEnv"
    - manglers
    - 
      psgi.url_scheme: { 'ENV': 'PSGI_URL_SCHEME' }
      HTTP_HOST: {'ENV': 'example.com'}
      SCRIPT_NAME: {'ENV': '/app'}

i.e. you can move all the configurations in the environment.