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

Make all routes relative to ROOT_URL to support reverse proxies #17

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

johipsum
Copy link

@johipsum johipsum commented Aug 5, 2015

Add ROOT_URL support.

Issues:

@inka
Copy link

inka commented Aug 25, 2015

+1

@bogdanrn
Copy link

bogdanrn commented Oct 9, 2015

Any news on this?

@awatson1978
Copy link

Can we get this PR merged in? It's much needed and appreciated functionality.

@awatson1978
Copy link

Any chance of getting a status update on this PR? We're needing to go live on a project with this functionality; and are debating whether we need to fork the packages or not. Thanks!

@MaximusDecimus
Copy link

+1

@chrisbutler
Copy link
Contributor

@johdirr @awatson1978 i think the functionality definitely makes sense, but are there any side effects of this change?

@awatson1978
Copy link

Don't know. That's what version numbers are for! ;)

@johipsum
Copy link
Author

i couldn't discover any side effects

@bogdanrn
Copy link

I'm really looking forward to this, i'm using OrionJS and they don't allow to change "/admin" path. Being able to setup the root_url will fix this annoying thing. Also i'm thinking this will help with other packages too.

In the mean time for whoever is waiting for something similar, someone pointed me towards a nice article about how to run meteor trough proxies from a subdirectory :
https://gist.github.com/soheilhy/8b94347ff8336d971ad0

@awatson1978
Copy link

So, I just forked iron:url, merged this PR in, and am not getting the prefix to work. I've tried the following syntaxes....

ROOT_URL_PATH_PREFIX=admin meteor
ROOT_URL_PATH_PREFIX="/admin" meteor
ROOT_URL_PATH_PREFIX="admin/" meteor

ROOT_URL=http://localhost:3000/admin meteor

@johdirr - Do you have a syntax example of how this is suppose to work?

@awatson1978
Copy link

Actually, just realized that all the routes are available on the ROOT_URL. It's Router.go() that's not using the ROOT_URL_PATH_PREFIX.

@awatson1978
Copy link

Wasn't able to get this full integrated. Wound up using the solution from PR #13 instead. Router with ROOT_URL support available here:

https://atmospherejs.com/?q=clinical%3Arouter

Command line installation includes:

meteor add clinical:router

And support for including in packages...

api.use('clinical:[email protected]');

@johipsum
Copy link
Author

johipsum commented Nov 4, 2015

@awatson1978 you don't need to set the ROOT_URL_PATH_PREFIX env variable. To make you're app available at http://localhost:3000/admin just run meteor with

ROOT_URL=http://localhost:3000/admin meteor

Meteor is setting the __meteor_runtime_config__.ROOT_URL_PATH_PREFIX variable for you.

I just tested my PR again with the newest meteor, iron:router, iron:url, ... versions and it works fine for me. Router.go() works too. What Errors do you get?

@awatson1978
Copy link

Hi. Wasn't getting errors. It was the Router.go() command didn't add the ROOT_URL_PATH_PREFIX to the route.

We adjusted the Router.prototype.go function to look at the Meteor.absoluteUrl() path to get the path prefix, since absoluteUrl sends the necessary ROOT_URL at the right time for the client side templates to parse it.

Router.prototype.go = function (routeNameOrPath, params, options) {
  var self = this;
  var isPath = /^\/|http/;
  var path;
  var routeWithPrefix = routeNameOrPath;

  if (Meteor.absoluteUrl().split('/').length > 4) {
    routeWithPrefix = "/" + Meteor.absoluteUrl().split('/')[3] + routeNameOrPath;
  }

  options = options || {};

  if (isPath.test(routeWithPrefix)) {
    // it's a path!
    path = routeWithPrefix;
  } else {
    // it's a route name!
    var route = self.routes[routeWithPrefix];
    assert(route, "No route found named " + JSON.stringify(routeWithPrefix));
    path = route.path(params, _.extend(options, {throwOnMissingParams: true}));
  }

  // let Iron Location handle it and we'll pick up the change in
  // Iron.Location.get() computation.

  Iron.Location.go(path, options);
};

https://github.com/clinical-meteor/clinical-router/blob/devel/lib/router_client.js#L229

I think the consideration is that we have an edge cases where our proxy server works a bit different than your proxy server.

@chrisbutler
Copy link
Contributor

@awatson1978 so we have a complete, working solution for this now?

@awatson1978
Copy link

More or less. We're moving the ckcc project into production behind a proxy server using clinical:router.

Right now we've got our app running green in our CI test environment, which doesn't use the route prefix from the ROOT_URL; but we're getting ready to do a live run with it behind the proxy server.

I suppose I could add some acceptance tests that check the prefix as well. Will need to think on that.

@chrlvclaudiu
Copy link

Thanks @awatson1978 . I ended up using clinical:router, too. my ROOT_URL var has the subfolder specified (e.g. http://domain.com/app) and meteor sets the correct path preffix.
I had some issues with configuring apache proxy using subfolder and for anyone interested here are my directives to successfully proxy to a subfolder.

[apache]
ProxyPreserveHost On
ProxyPass /app/ http://ip_address:3002/app/
ProxyPassReverse /app/ http://ip_address:3002/app/

[meteor]
export ROOT_URL=http://domain.com/app meteor

@abate
Copy link

abate commented Sep 7, 2016

so the accepted solution is to switch to clinical:router ? I'm in the same boat the the OP using ngnix ...

@methodbox
Copy link

I no longer use Meteor, having opted for my own templating and build system using Vue, Mongo and Express.

However, last I used Meteor, clinical-router did solve this issue. Flow router may also address this, but not sure.

@Steditor
Copy link

Any updates on this PR?

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

Successfully merging this pull request may close these issues.

10 participants