Fails to find routes when using Router about get-routes HOT 41 CLOSED

Ello, looked a little more into this. Found another scenario, and just want to place it here as a log of the issue. I am working on it bit by bit. Though I am afraid there is not a lot of the original code left. :(

app.route("my-route")
    .get(myRouteGet)
    .post(myRoutePost);

from get-routes.

Just to keep you up to date.. I was doing fine until I hit

router.get("/level2");
app.use("/level1", router);

It would only list /level2 and not /level1/level2

And as a secondary note. Not actually a problem since I plan to filter duplicates. But if someone wanted to list the order of presidency, it cannot be done with all the methods being split.
Like I said, I am not concerned with this.. Just a note.

from get-routes.

I write a path to fix this one, just waiting for my previous PR

from get-routes.

@RaianRaika This would be awesome 👍

I'm very looking forward to it, and if you have any questions feel free to ask 😊

Regarding the test names: Please don't relate them to the issue or the issue's number, but rather use something descriptive (in the same style as the other tests).

from get-routes.

Okay, makes sense. So, if you want to, feel free to submit a PR 😊

from get-routes.

Thanks for bringing up this issue 😊

Would you mind sending a PR?

from get-routes.

I was about too when I thought of extra issues related, and now I am getting quite deep and not been able to solve it yet..
Problems so far:

1. app.use("/path", router) doesn't account for "/path/test"
2. nested router within router. And cannot recursively call getRoutes, so may need to do some custom thing for recursive routers.
3. nesting a full app within another app or router.. (Yes, sadly this is possible).

from get-routes.

Unfortunately, I (currently) have not that much time left to care about this, but if you can come up with a working PR, I'll happily merge it 😊

from get-routes.

@WORMSS Thanks for the update, and that sounds reasonable and great at the same time 😊

from get-routes.

Closing this since there has been no more activity for 6 months now.

from get-routes.

@goloroden

I want to start working on this issues, for starting, I'll add a unit test named issues01 or something like this.
is it acceptable?

from get-routes.

Hi @goloroden

I implement a recursive function to extract express nested routes, and it works for simple routes.
but in routes that has in path param need your help, express route, use a regex to detect in path params, what's ur idea to show it in our output?
show the regex, or replace the regex with the real param name.

from get-routes.

Can you give an example of what the output would look like in both cases?

from get-routes.

For example, I define a route like this:

    // Defined a 3rd level router.
    routerL3.post('/nested', (req, res) => res.send());
   // Defined a 2sc level router.  
    routerL2.use('/(:inPathParam)/level2', routerL3);
    // Define an app route.
    app.use('/topLevel', routerL2);

The defined route is :
/topLevel/12/level2/nested

That 12 is a param that passed in URL path.
This URL(route) config in the express app ,is stored like this:
'/topLevel/(?:(?:([^\/]+?)))\/level2/nested'

from get-routes.

To be honest, I didn't quite understand your sample. May it be that some formatting went bonkers?

from get-routes.

Well, I'm not sure, but maybe it happens in some usage of the module, In my opinion, its better module return the human-readable URL because as I know in other related modules, they accept that kind of format.

Anyway, I add more details to my example, please check, I hope to be clear it now.

from get-routes.

OKay, finally I implement it in both ways, for anyone who needs each of this.

I'll add a config to the get-routes constructor to switch between regex or human readable URL forms. and set the default to human-readable, is it okay?

from get-routes.

If you have two parameters, one of type human readable url (which essentially means string), and another one of type regex, there shouldn't be any necessity to have a configuration which says which one to use. Simply create one constructor and separate by type.

from get-routes.

Hi @goloroden,

Merry Christmas.

It's a good idea, so I sperate this two on the constructor,
what's ur Idea for the constructor name? and which one be the default?

from get-routes.

Merry christmas to you, as well 😊

When you said:

I'll add a config to the get-routes constructor to switch between regex or human readable URL forms.

Which function did you have in mind? Are you referring to an existing one with "the get-routes constructor"? If so, the name should already be given, shouldn't it?

Regarding your question what should be the default one: Why is that important? If the function is able to detect the type of parameter, why does something such as a default exist at all?

from get-routes.

Well, I think I describe it a bit messy 👿
Let categorize the things to address the options:

1. Use a config argument in the get-routes constructor to switch between human-readable and regex form of URL parsing.( this one implemented right now )
2. Use 2 constructors to create get-routes objects with different URL parsing behaviors.

The first question is, which one is better in your opinions?

from get-routes.

Hi @RaianRaika!

Thanks for explaining things a little bit more structured, I now have a better idea of what you want to accomplish.

However, there is one thing I still don't get: Why do you need this distinction at all? Shouldn't it be transparent to the user? In other words: Why should I (as a user) even care how the routes have been created?

What is the task you try to accomplish where you need this information?

from get-routes.

Hi @goloroden,
Well, good question ...
it depends on why a user wants to use this module, in my case, I need the human-readable URL, the form that developer create the routes. but the URL is not stored as defined URL in an express app.
so I try to address my issue and need the real defined URL ( human readable form ) by the developer in the app.
I hope it is clear now.

from get-routes.

Hi @RaianRaika!

Happy new year 😊

Okay, we're getting somewhere … I now better understand your point of view. Finally, one last question: Can you provide an example that demonstrates the behavior? And point out what the actual vs the expected behavior is?

from get-routes.

HI @goloroden
Happy new year 😊

Well, let first determine why I need this behavior:
I am on developing a module that tries to validate params on middleware level for each request in Sails.js and Express.js apps, in my scenario, the developer defines routes and their params in a JSON file, and my module read it and extract the params from the request and execute the defined rules for each incoming requests. for matching rules and incoming request, my module needs to determine the routes from the web app ( sails or express ) and match them with routes that defined in JSON files by developers.

In an Express.js app, the routes that define with Router or the routes that have in path param, parsed and stored in the web app as a regex.
For example, if an Express.js app defined like this :

    const app = express();
    const routerLevel1 = new Router(),
          routerLevel2 = new Router();
    const nestedRouter = new Router();

        // Simple route.
    app.get('/(:inPathParam)', (req, res) => {
      res.send();
    });

The routes in runetime stored like this in App Object

/^\/(?:(?:([^\/]+?)))\/?$/i

but I need the routes in human readable form, something like this :

/(:inPathParam)

so for handling this different, if somebody needs the human-readable form of routes need to call the get-routes module in this form
const routes = getRoutes(app, { humanReadable: true });
and if need it as regex just call it without any config param.
const routes = getRoutes(app);

I hope it is clear now.

from get-routes.

Thanks a lot for this very detailed explanation, I now (finally) got it 😊

I think that most people define routes in the human-readable form when creating an app, so I'd expect getRoutes to return the human-readable form by default as well.

This brings up the question how to call the function when you want the regex form. To be flexible and open for future additional formats (whatever they will be), what about:

const routes = getRoutes(app);
// => Human-readable

const routes = getRoutes(app, { format: 'regex' });
// => Regex

Theoretically you could also call

const routes = getRoutes(app, { format: 'human-readable' });
// => Human-readable

but this does the same as when omitting the parameter. So, to cut a long story short, the signature of the function should be:

getRoutes (app, { format = 'human-readable' }) {
  // …
}

Does this work for you?

from get-routes.

Yep, it works like a charm 👍
I'll update my code and create the PR by today.

from get-routes.

This is a little long for an issue but thought I'd put this out here. I rearranged a thing or two and removed the Sails stuff and made this version. It only supports a single layer of Router (as noted) but seems to be doing the trick for me and my needs.

const supportedMethods = [ 'get', 'post', 'put', 'patch', 'delete' ];

const getRoutes = function ( app ) {

  if ( !app ) {
    throw new Error( 'App is missing.' );
  }

  const routes = {};
  supportedMethods.forEach( supportedMethod => {
    routes[ supportedMethod ] = [];
  } );

  /***********************************************************
   APP usage
     this will pull out the routes configured using this syntax
       // index.js  
       const express = require( 'express' );
       const app = express();
       app.get('/')
  *************************************************************/

  const appRoutes = app._router.stack.filter( stack => stack.route );

  appRoutes.forEach( route => {
    const method = route.route.stack[ 0 ].method;
    const path = route.route.path;
    routes[ method ].push( path );
  } );

  /***********************************************************
   ROUTER usage
     this will pull out the routes configured using this syntax
       // childRoute.js
       const express = require( 'express' );
       const router = express.Router();
       router.get('/')

       // index.js  
       const express = require( 'express' );
       const app = express();
       app.use( '/child', childRouter )
     
     NOTE: this only works for a single layer of router importing
       if you are importing routers into anothe router
       and then importing that into the app this will not follow that recursion
  *************************************************************/

 const routerRoutes = app._router.stack.filter( stack => stack.name === 'router' );

 routerRoutes.forEach( route => {
   const parentPath = route.regexp.toString().replace( '/^\\', '' ).replace( '\\/?(?=\\/|$)/i', '' ).replace( /\\\//g, '/' );
   route.handle.stack.forEach( route => {
     const method = route.route.stack[ 0 ].method;
     const path = parentPath + route.route.path;
     routes[ method ].push( path );
    } );
} );

  return routes;
};

module.exports = getRoutes;

from get-routes.

@arsdehnel If I understand you correctly, your code solves the issue, but removes Sails support, right?

from get-routes.

@arsdehnel , Also It not supports nested route definition in express.

from get-routes.

@goloroden solves the issue to return the same results as the current master implementation for a single-level Router setup (or a combination of a single-level router and single-level route definition. The master branch doesn't handle this either so I just added the Router option into here and then removed Sails as I have no familiarity with Sails nor did I have a need to continue the support. Not sure if Sails has the concept of a Router or anything so it might be as easy as putting my code into the isExpress block and that being a good improvement?

@RaianRaika you're right that this doesn't solve nested route definition. As I mentioned I don't need that and the current master branch doesn't support it so that was intentional to leave that out.

from get-routes.

@goloroden I can create a PR (if you want one) but not sure what sort of setup i need for running the tests or anything.

from get-routes.

@arsdehnel @RaianRaika I have been thinking about this for a few days now.

I think it's difficult to find the right way to solve this issue, since there are some pros and cons:

• On the one hand I would love to merge a PR that solves this issue.
• I understand that it's difficult to adopt the same changes for Express and Sails in the same way.
• We ourselves don't use Sails as well, so it's becoming harder to maintain this the more features this module gets.
• On the other hand, @RaianRaika has put a lot of effort into integrating Sails support.

Right now, I think, the best solution would be to split the module into two modules, one for Express, one for Sails. This way both modules become easier to maintain, and both can have the features suitable for the appropriate framework.

What do you think about this?

from get-routes.

Hi @goloroden ,

Well, this problem is not about the sails, because sails had an isolate routing service,
this issues always just happened in express, because it's pluggable with the router.
for solving this issue I write my path but have not the time to remove extra things from it to match your rules for accepting PR.
another suggestion is just re-try by this weekend and send my path if you want can accept it,
it has the support of fixing the issue, and support for both Sails and Express.jS and also it supports nested router configuration in express.

from get-routes.

Okay, I see…

But anyway: I think it would be way easier (in terms of maintainability) if we split this module into two, one for Express, one for Sails (especially since you most probably won't have both at the same time in one project).

from get-routes.

This one is small module by itself, so I agree to split it into two submodules, but not a totally separated module.

from get-routes.

Really good 😅
Can u give me an explanation of what kind of separation do you agree on it?
and if u be agreed, I can clean my prev PR and add to the new one?
I'm thinking about sperate them from the Entry point, do you agree with this?

from get-routes.

What about two separate entry points, so you have to require either get-routes/express or get-routes/sails?

from get-routes.

Makes sense to me.

from get-routes.

Hi @goloroden ,
Sorry for a long time,
just now find a bit time to push this PR again

about the separate the entry point for Sails and Express,
Unfortunately, I cannot run the build task with the bot, so I just skip it for now, please help me on this to resolve this part

from get-routes.

Closed, since we don't actively maintain this repository any more (please consider it as deprecated).

from get-routes.