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

Fix special case where "/" would match with all paths #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rgigger
Copy link

@rgigger rgigger commented Sep 12, 2020

I noticed that if I put my "/" route first that it would match with all paths, so I had to list it as the last path in order for anything to work. I am not sure if adding another special case rule is the best way to fix but it does seem to do the job. This causes MacroExpress to now match the behavior of exress.js in the same situation.

@helje5
Copy link
Member

helje5 commented Sep 13, 2020

I think Macro currently does a prefix match on all calls. So I think we would rather want to change that if Express.js does exact matches?

@helje5 helje5 added the question Further information is requested label Sep 13, 2020
@rgigger
Copy link
Author

rgigger commented Sep 14, 2020

Yes, I think that's right. Currently Macro does a prefix match in the sense that a route using the pattern '/cows' will match with a path of '/cows/list' but it won't match with say '/cowss'. But yeah the express behavior is to not match either of those. I will generalize it do exact matches everywhere and update the PR.

@helje5
Copy link
Member

helje5 commented Sep 14, 2020

I recently added some "route" tests, would be cool to have those in sync as well and test the desired behavior.

I wonder whether prefix matching '/cows' is currently required for mounting, or whether that needs special behavior then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Development

Successfully merging this pull request may close these issues.

2 participants