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

Allow users to have an additional regex node #317

Closed
ivan-tymoshenko opened this issue Feb 5, 2023 · 10 comments · Fixed by #320
Closed

Allow users to have an additional regex node #317

ivan-tymoshenko opened this issue Feb 5, 2023 · 10 comments · Fixed by #320

Comments

@ivan-tymoshenko
Copy link
Collaborator

ivan-tymoshenko commented Feb 5, 2023

find-my-way has a caveat: it can't have two different routes which differ only for their parameters. What we can allow users to have is a regular parametrical node and a regexp node (or multi-parametrical node, which is also a regexp node), in case there is only one regex node. In this case, we know that the regex node has higher priority than the regular one.

Example:

  • /foo/:param1
  • /foo/:param2.png

@mcollina @Eomm @Uzlopak @RafaelGSS @climba03003 WDYT?

@ivan-tymoshenko
Copy link
Collaborator Author

I have some doubts about that #314 (comment). I'm not sure that it's possible.

@mcollina
Copy link
Collaborator

mcollina commented Feb 5, 2023

This is doable and it would allow us to cover more cases.

@ivan-tymoshenko
Copy link
Collaborator Author

How would you resolve cases like this #314 (comment)?

@Eomm
Copy link
Contributor

Eomm commented Feb 5, 2023

I think it is a matter of strictness and execution order (from left to right) over priority

Taking the examples

  • /foo/:param1: handler 💎
  • /foo/:param2.png: handler 💡
  • /:lat-:lng/bar/:coords: handler 🌲
  • /:coords/bar/:lat-:lng: handler 🔥

I would expect

request result notes
/foo/asd 💎
/foo/asd.png 💡
/foo/asd.foo 💎
/foo-bar/bar/foo-bar 🌲 Because :lat-:lng is a stricter match over :coords and I would say that the URL's leftmost path has more "weight" over the rightmost part

@ivan-tymoshenko
Copy link
Collaborator Author

@Eomm What do you think about case like that? Can we say that the node with a static ending has higher priority than a regexp one?

  • /foo/:param1(.*)
  • /foo/:param1(.*).png

@Eomm
Copy link
Contributor

Eomm commented Feb 12, 2023

What do you think about case like that? Can we say that the node with a static ending has higher priority than a regexp one?

I would say: yes, but - may it be a user error?

@ivan-tymoshenko
Copy link
Collaborator Author

ivan-tymoshenko commented Feb 12, 2023

That is a part of the question. If we think that those routes have the same priority, then we should throw an error. It's basically a question of param nodes' static endings. In some cases, we want to allow users to have two similar nodes with different static endings in the param.

This is working now. We don't need to prioritize them, because it's impossible to match them both:

  • /foo/:param1(.*).jpeg
  • /foo/:param1(.*).png

The idea of this issue is to add support for routes like that. In this case, we can tell that second route has higher priority than the first one:

  • /foo/:param1
  • /foo/:param1.png

The question is: should we go further and allow users to have:

  • /foo/:param1(.*)
  • /foo/:param1(.*).png

But make sure that we don't care about regexp value. So this would also work if we allow it to work:

  • /foo/:param1([a-zA-Z])
  • /foo/:param1(.*).png

@climba03003
Copy link
Contributor

Can we draw a line like more specific chunk should match first at the same level.
The chunk means the value between /.
Same level means the types for each start of chunk.

For example,
/(regexp)(static) and /(regexp). The first one will take higher priority since regexp with static is more specific.
/(regexp)(regexp) and /(regexp). The first one will take higher priority since two regexp is more specific.

Or even
/(regexp)(static)(regexp) and /(regexp)(static). The first one will take higher priority.

@ivan-tymoshenko
Copy link
Collaborator Author

ivan-tymoshenko commented Feb 12, 2023

Can we draw a line like more specific chunk should match first at the same level.
The chunk means the value between /.
Same level means the types for each start of chunk.

The name of the chunk is node.

We have three different node types: static, parametric, and wildcard. Static node always has the highest priority, wildcard node - the lowest. Everything between them is a parametric node.

Parametric node always starts with a : (colon) char. Logically we can define three types of parametric nodes:

  • /:param - general parametric node. Has the lowest priority within parametric nodes.
  • /:param(.*) - regexp node.
  • /:param1(.*)-static.:param2 - multi-parametrical node. Can be any combination of params, regex params, and static parts. IMPORTANT: multi-parametrical node exists only from the user's point of view. In reality, we convert it to the regexp node.

/(regexp)(static) and /(regexp). The first one will take higher priority since regexp with static is more specific.

I might agree with that. It's a part of my question.

/(regexp)(regexp) and /(regexp). The first one will take higher priority since two regexp is more specific.

I disagree with that. Make sure that all regexp in your example can be different:

  • /:param1(.*)-param2(.*)
  • /:param1(123-456)

I don't see why the first node should always have higher priority. Feel free to change my mind.


/(regexp)(static)(regexp) and /(regexp)(static). The first one will take higher priority.

I think completely opposite. The first route in your example is a regexp node from the logical point of view. It can be unclear, but we can treat it only as one big regexp like that ^(regexp)static(regexp). And don't forget that we can't compare the regexp values.

Example to show another point of view:

  • /:param1(.*).param2(.*)
  • /:param1(.*).png

Don't forget that we can say that comparing nodes like that is incorrect and keep thowing an error like we always did.

@climba03003
Copy link
Contributor

climba03003 commented Feb 13, 2023

I don't see why the first node should always have higher priority.

Since regexp complexity cannot be measured or compared.
I would say that we can only compare the complexity of node. So, that's why I would prefer 2 regexp is higher priority than 1 regexp.

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

Basically, it would be the reverse order of 4. and 5.

Don't forget that we can say that comparing nodes like that is incorrect and keep thowing an error like we always did.

I don't mind if it continue to throw at this point. And update the behavior when we have more consensus.

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

Successfully merging a pull request may close this issue.

4 participants