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

Ability to add non dynamic parameters in a route #821

Closed
neolao opened this issue Sep 4, 2013 · 22 comments
Closed

Ability to add non dynamic parameters in a route #821

neolao opened this issue Sep 4, 2013 · 22 comments

Comments

@neolao
Copy link

neolao commented Sep 4, 2013

For example:

module.exports.routes = {
    "/a": {
        controller: "MyController",
        action: "index",
        parameters: {
            type: "foo"
        }
    },
    "/b": {
        controller: "MyController",
        action: "index",
        parameters: {
            type: "bar"
        }
    }
}
@particlebanana
Copy link
Contributor

Will query parameters not work for you? So given:

module.exports.routes = {
    "/a": {
        controller: "MyController",
        action: "index"
    }
}

/a?type=foo and /a?type=bar would give you the same result as the parameters above.

@neolao
Copy link
Author

neolao commented Sep 4, 2013

No, the urls are not the same.

  • /a uses the controller MyController, the action index and the parameter type with the value foo
  • /b uses the controller MyController , the action index and the parameter type with the value bar

@reecelewellen
Copy link
Contributor

Is adding a request parameter to the URL an option? eg /a/:type and /b/:type that way the same controller could parse the param via req.param('type')?

@neolao
Copy link
Author

neolao commented Sep 4, 2013

I know the feature, but I don't want dynamic parameters.
I want to use the URLs http://mywebsite.com/a and http://mywebsite.com/b.

@reecelewellen
Copy link
Contributor

Could you parse req.route.path?

I just did a test in my controller to console.log that attribute and it showed /path/to/my/controller

@reecelewellen
Copy link
Contributor

I assume your controller could be something like

if(req.route.path === '/a'){//do stuff})

@neolao
Copy link
Author

neolao commented Sep 4, 2013

It is what I did actually, but it's not nice.
If I need to parse the url in the controller, it is not really decoupled with the routing.

@sgress454
Copy link
Member

I guess I don't quite understand the use case. If you want two different routes to do two different things, why not write two different controller actions for them instead of having them both use "index"? If you need them to share some functionality, you can use services.

@neolao
Copy link
Author

neolao commented Sep 4, 2013

For example, I have 10 custom urls for landing pages. They are the same but with some variations.

@reecelewellen
Copy link
Contributor

A policy could do multiple little tweaks off the request.route and then
pass adjustments to the controller. Or as sgress said a service could do
lots of the same functions and you could add the service to multiple
controllers?

@sgress454
Copy link
Member

Or you could still use dynamic parameters for this. Putting this at the bottom of your routes file:

"get /:type": "MyController.index"

will give you access to "req.param('type')" in your action. Of course, this route matches everything, and will override blueprint actions for models. To get around that, you can either check the value of "type" in your action method against the list of allowed landing page types and do return next() if there's no match, or prefix your different landing page types with an underscore (or some other character sequence):

"get /_:type": "MyController.index"`

so that going to "/_a" will trigger the action and req.param('type') will return "a".

@neolao
Copy link
Author

neolao commented Sep 4, 2013

I think the policy is the best choice but it's weird in all cases.
Php frameworks do that (it's not a troll).

@neolao
Copy link
Author

neolao commented Sep 4, 2013

My request is really invalid ?
You don't understand the use case ?

If I use a policy, I will parse the url too, it's bad.

@sgress454
Copy link
Member

No offense intended. Your use case is understood--we've just given you several options for how to do this. I'm not sure what it is you don't like about dynamic parameters, but they exist to give you the functionality you're asking for. The idea is to keep logic out of the routes config file where it doesn't belong. If you want to have an arbitrary number of routes all doing the same thing with minor variations, you can have one route for the whole lot, and then manage the differences in the Controller. If you were managing a membership site with vanity URLs, for example, you'd want to manage the custom URLs (aka slugs) in a database, not in routes.js.

@neolao
Copy link
Author

neolao commented Sep 4, 2013

I'm ok with dynamic parameters but as you said, my urls are in the root of the website and it's problematic.
It's like slugs but for some predefined urls (for SEO). I will not put them in database.

I want to manage the differences in the controller but I don't want to parse the URL because it is the responsability of the router.

@sgress454
Copy link
Member

As long as you put the route at the bottom of your routes.js file, using dynamic parameters is only an issue for model blueprints, or if you're serving static assets from the root (don't do this). The blueprint problem is easily handled, either in your routes.js by explicitly writing the blueprint routes:

// Assuming a model "user"
"get /user": "UserController.find"

or in your Controller, especially if you have a predefined set of values for "type", as you do:

index: function(req, res, next) {

  var type = req.param('type');
  if (!isValidType(type)) {return next();}
  // Do your thing...

}

function isValidType(type) {
  // Check against list of valid types...
}

@neolao
Copy link
Author

neolao commented Sep 4, 2013

Ok, it's a solution but it seems like a trick.

Even if I send a pull request to handle default/static parameter values, you will not accept it, right ?

@particlebanana
Copy link
Contributor

The router in Sails is just the router from Express. It's purpose is to parse the URL based on a set of regular expressions and find a matching function to handle it.

The wildcard route above: 'get /:type': 'MyController.index' is the correct way to do it. This will give you access to the type attribute you want. You simply define a handler and it will be exactly like your initial example. You will need to handle the case where someone types a page that doesn't exist and send them to a 404 but that would need to happen anyway.

This gives you:

http://www.mywebsite.com/a as well as http://www.mywebsite.com/b which both map to the same controller where you have access to req.param('type').

@sgress454 solution works great and doesn't seem like a trick at all. You are simply checking if it is a valid landing page or not and if not sending it off to see if the request matches anything else in the middleware stack.

@neolao
Copy link
Author

neolao commented Sep 5, 2013

I don't see a or b as parameters of a controller in my use case. The path /c can have the same variation as /a (ex. canonical url). I want to configure the controller from the router in order to serve the same variation for /a and /c. It is not the responsability of the controller to do that.

And if I want /foo to be handled by an another controller because the nature is different ?

Imagine the urls are localized. I don't want to use a switch in the controller with all the possibilities for languages.

@particlebanana
Copy link
Contributor

Alright I have a better idea of what you are trying accomplish now. I thought about it and came up with some code I think will make everyone happy. We don't have to mess with the router and you get a nice dictionary of routes that map to a controller with a key.

https://gist.github.com/particlebanana/6445791

Using the example of routes by language I put together this gist you can play with. It uses a dictionary file that functions similar to the way i18n works but without the policy to detect region. You can add that later if you want.

You can map out pages to a controller and action and then add all the various language translations in a single place. If you look at languageMap.js you can see what I mean.

Then in the bootstrap.js file we use the language map to dynamically inject the routes into the router. So no more dynamic parameters and no wildcard routes. You can attach as many keys to the req object as you like.

I know it's not your ideal solution but let me know what you think after playing with it a bit. You can just paste that file into bootstrap.js and add your language map and it will work.

@neolao
Copy link
Author

neolao commented Sep 6, 2013

I don't doubt there are several solutions. My example of the localized urls is a special case I think.

I don't want to skip the routes.js. It's a configuration file to route URLs to controller actions.
The controller uses parameters and doesn't know about the URL. In most cases, we use something in the URL as parameter but it's not an obligation. It's the router who decides to use the part of the URL as a parameter of a controller action.

A solution where I have to parse the URL is not nice because it is the responsability of the router.

@Aidan-Chey
Copy link

For anyone coming accross this for a solution. This is completely possible now. Like the original request states, add the data to the route's object like so

"myRoute" : { 
    controller: "myControler",
    action: "myAction",
    customData: anythingHere
 }

Then, in the controller you can access the data from req.options, for example: let dataFromRoute = req.options.customData;

I havn't come accross this personally but i'd make sure the key you are putting your data under dosn't clash with any existing data in the options object.

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

No branches or pull requests

5 participants