Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Routing customization, mount home page to "/" #232

Open
ecbrodie opened this issue Feb 12, 2017 · 6 comments
Open

Routing customization, mount home page to "/" #232

ecbrodie opened this issue Feb 12, 2017 · 6 comments
Labels

Comments

@ecbrodie
Copy link
Contributor

It would be great if there were more options available for customizing the routes that flipper-ui can mount to.

As of present, the route for the home page is defined as "/features" (see https://github.com/jnunemaker/flipper/blob/81e92592c499005aabf42a7ffbe32ba7582859aa/lib/flipper/ui/actions/features.rb#L9). This means that whatever path I mount the flipper-ui routes to in my Rails application's routes.rb file, the home page is at least forced to have a URL path of "/features" appended to it. Mounting the Flipper::UI app to "/" will not work because this will affect my app's other routes, so I am forced to choose another URL path to mount the UI app to. Thus, at the very least the URL of the Flipper UI home page must be "/my-custom-path/features".

I think that there should be more routing customization available for users of flipper-ui. At the very least, routing the home page to "/" would be nice; it allows my app to mount the Flipper::UI app to "/features" and open the home page at "/features" instead of "/features/features". Otherwise, if there must be a non-root route name to route the Rack actions to, perhaps adding an options Hash parameter to the contract of Flipper::UI.app() could enable customization of the route names.

@jnunemaker
Copy link
Collaborator

I think it totally makes sense to drop "features" and just have "/" be the main route if we can make it work. I can't remember why I did it the way I did. The only thing I can think is that I wanted something to match against that felt moderately unique to the flipper UI hence "features". If we anchor all the regexes to the end, remove the home action and make the features action last, I think it will just work without features being in there. Open to a PR for sure. I'll try to tackle it at some point if you or someone else doesn't have time. Thanks for taking the time to write this up!

@AlexWheeler
Copy link
Collaborator

@ecbrodie you should be able to mount Flipper::UI at / as long as you make sure you mount the app after all of your other routes. This is because mount will try to match any routes after the pattern you mount the rack application (Flipper::UI) at. So if you mount it at / its going to route every request to Flipper::UI. However, if you mount it after all your routes (and you have no conflicting routes, when it doesn't find any routes defined in your rails app, it will make it to Flipper::UI and use those routes, which is what you want.

# good
Rails.application.routes.draw do
  get '/welcome', to: 'welcome#index'
  post '/welcome', to: 'welcome#create'
  
  mount Flipper::UI.app(flipper) => '/'
end

# bad
Rails.application.routes.draw do
  mount Flipper::UI.app(flipper) => '/'

   get '/welcome', to: 'welcome#index'
  post '/welcome', to: 'welcome#create'
 

Does that work for you?

@AlexWheeler AlexWheeler added the ui label Feb 14, 2017
@AlexWheeler
Copy link
Collaborator

the caveat being that you couldn't have anything else resolve to '/' which would be a problem if you wanted to have a different root. So yea in that case this could be sweet to implement

@AlexWheeler
Copy link
Collaborator

AlexWheeler commented Mar 24, 2017

Started looking into this so thought I'd add an update for anyone else that is interested:

The part that makes it a little tough is making sure that static assets don't match a flipper route. When we remove the /features/... from the routes it makes it easier for a stylesheet for example to match as a route, and the headers get set to 'text/html', instead of 'text/css'.

The UI Middleware serially checks if the request url matches a defined action. If it doesn't match any of the flipper entities (features,gates, etc) it checks to see if it is a static asset request, for example for a stylesheet. Assets don't live under /features/ so we'll never have an issue.

        # UI
        @action_collection.add UI::Actions::Features
        @action_collection.add UI::Actions::AddFeature
         ....
        # Static Assets/Files
        @action_collection.add UI::Actions::File

An example:
https://github.com/jnunemaker/flipper/blob/master/lib/flipper/ui/actions/feature.rb

changing route %r{features/[^/]/?\Z} to route %r{/[^/]/?\Z} now all stylesheets, javascripts etc will match as feature requests and get returned with content-type 'text/html'.

for example:
%r{features/[^/]*/?\Z} won't match "/application.css"
%r{/[^/]*/?\Z} matches "/application.css"

Anyways, definitely a fun problem to solve. I'll keep looking into it and think of the best place to put the logic to make sure asset requests don't conflict with other actions

@jnunemaker
Copy link
Collaborator

@AlexWheeler thanks for digging in and writing up the results. Super helpful. After looking at this again a few days ago, I did realize that we redirect the root to /features, so I don't think this is a huge problem. It feels only aesthetic really, unless I'm misunderstanding something. If it is too much effort, I'm fine with leaving it as is. Also, I'm totally fine with changing some routes, like putting assets under /assets or whatever if that makes it easier.

@AlexWheeler
Copy link
Collaborator

AlexWheeler commented Mar 24, 2017

@jnunemaker one solution that seems to work well would be to register the File action first, so that would match before any of the other actions

        @action_collection = ActionCollection.new

        @action_collection.add UI::Actions::File

        # UI
        @action_collection.add UI::Actions::Features

would just need to think about it and make sure that wouldn't conflict with a user's feature name. But, it shouldn't be an issue since a feature shouldn't match

route %r{(images|css|js|octicons|fonts)/.*\Z}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants