Regex issue with other wildcard routes

[subhero24]

I encoutered an issue with a wildcard route that is not activated if I use a regex in another route.

When I have two routes /a and /* everything is fine and /b will render the wildcard route.
When I change the first route to use a regular expression to match "a", so that I have two routes /:a(^a) and /*, the second route will never render and /b will result in a 404.

const http = require("http");
const router = require("find-my-way")();

router.on("GET", "/:a(^a)", (req, res, params) => {
	res.end("a");
});

// This will not be used with a request to /b ??
router.on("GET", "/*", (req, res, params) => {
	res.end("*");
});

const server = http.createServer((req, res) => {
	router.lookup(req, res);
});

server.listen(1234, (err) => {
	if (err) throw err;
	console.log("Server listening on: http://localhost:1234");
});

[ivan-tymoshenko]

Hi, nice finding. Would you like to create a PR to fix it?

[subhero24]

Will try to create a PR for this.

As I am looking through the code, I have another question: is it expected behaviour to throw when defining two routes like this? Is only one regex allowed for every url part?

findMyWay.on("GET", "/:a(a)", () => {});
findMyWay.on("GET", "/:b(b)", () => {});

[ivan-tymoshenko]

https://github.com/delvedor/find-my-way#caveats

[ivan-tymoshenko]

The bug that you are trying to fix here:

find-my-way/index.js

Line 471 in a59117d

return null

[ivan-tymoshenko]

@subhero24 Do you need help with it?

[subhero24]

@ivan-tymoshenko

But should the following be allowed according to the caveats?

findMyWay.on('GET', '/:a(a)', () => {})
findMyWay.on('GET', '/:b(b)/static', () => {})

Cause matching this to /b/static fails

[ivan-tymoshenko]

Hmm, nice finding. I don't know yet. There is definitely a bug. Either we should throw an error when setting the second route or support that.

When it is a non-regexp parametric node, it's just one node. So in this case one regex node rewrites another.

[ivan-tymoshenko]

Can you open a separate issue for that?

[subhero24]

I am not that familiar with the code, but it seems the radix tree nodes have a parametricChild attribute. As it is not an array, i'm guessing two parametric segments in the url at the same position is not possible, regardless of the following route structure?

[ivan-tymoshenko]

Yes, you are right. But we always can change the tree structure). The question is can we support multiple parametric nodes? My first answer is no because we don't know how to prioritize them.

[subhero24]

It would seems very limiting to not be able to do this.

Do we need to prioritise them? could we keep these route parameters in the order they are registered?

[ivan-tymoshenko]

We shouldn't prioritize them by insert order. It can seem like a good idea for small projects, but it would be a disaster if you have a bunch of microservices.

[subhero24]

Maybe prioritise the more specific routes first (ie more segments vs less segments, more static segments vs parametric segments, etc)?

[ivan-tymoshenko]

IMHO, we can't say that one route is preferable to another using anything other than the length of static prefix or node type.

What we can do is separate the parametric node on the parametric node and regexp parametric node and handle the regexp parametric node before the parametric node. It will allow us to add one parametric node and one regexp node. But only one. IDK, if it is worth it.

[ivan-tymoshenko]

Can you open a new issue for this bug and describe it there?