Skip to content

Fix routing precedence algorithm#328

Merged
athoscouto merged 6 commits intomasterfrom
fix/routing-precedence
Jun 26, 2019
Merged

Fix routing precedence algorithm#328
athoscouto merged 6 commits intomasterfrom
fix/routing-precedence

Conversation

@brunoabreu
Copy link
Contributor

This PR makes our routing precedence algorithm work like the server-side one, implemented in C#

It fixes inconsistencies between client-side and server-side navigation.

The new implementation is based on this code: https://github.com/aspnet/AspNetWebStack/blob/master/src/Common/Routing/RoutePrecedence.cs

@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Jun 21, 2019

Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖

Please select which version do you want to release:

  • Patch

  • Minor

  • Major

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

  • No thanks, I would rather do it manually 😞

Copy link
Contributor

@athoscouto athoscouto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I could grasp, your algorithm is a little bit different from the ASP.NET one.
If two or more routes draw on the precedence, ASP.NET will count the number of matched parameters and use it to decide which route should match the URL.

This is where the witchcraft happens.

For example let the URL be /x/y/z be the URL, even though routes /*a/x/*b/ and /*a/y/*b have the same precedence, /*a/y/*b/ should be the one to match the URL, because *a would match some not empty subpath only on the second route.

Even though I'm stating it works like this, I'm really not confident on my C#, so I may be metendo os pés pelas mãos.

Another detail (here) is that when it cannot decide between two routes, it throws an error. This blog says that:

And that's it as we don't have any ties to break, but if a tie between two routes did exist, then a case-insensitive comparison of the route template names would resolve it.

But I haven't found anything on code that would back that up.

Other than this questions regarding the equivalency of the algorithms, there is one minor issue with the code.

Copy link
Contributor

@thiagomurakami thiagomurakami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. It looks much simpler than before :D
LGTM

@thiagomurakami
Copy link
Contributor

thiagomurakami commented Jun 22, 2019 via email

@brunoabreu
Copy link
Contributor Author

@athoscouto you're right. The new algorithm doesn't perfect match the server-side one.

I created this issue #330

For now this PR will fix some navigation bugs we are experiencing but we can come back to this issue later.

@brunoabreu brunoabreu force-pushed the fix/routing-precedence branch from 1fce2b5 to c65eb12 Compare June 26, 2019 18:26
@athoscouto athoscouto merged commit 8b78d9b into master Jun 26, 2019
@vtex-io-ci-cd
Copy link
Contributor

vtex-io-ci-cd bot commented Jun 26, 2019

Your PR has been merged! App is being published. 🚀
Version 8.38.3 → 8.39.0

@athoscouto athoscouto deleted the fix/routing-precedence branch June 26, 2019 22:07
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.

4 participants