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

Avoiding dynamic parameters from previously tested routes #27

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

Conversation

vincenttheeten
Copy link

While scanning for routes to match, a params dictionary is built up.
Even if the route does not match in the end, the params (name, value) are kept.
I don't think this is intended functionality, so I fixed it.

@mtrpcic
Copy link
Owner

mtrpcic commented Jun 8, 2012

I may be missing something that you're seeing, but since there was no match, the params never get used. Thus they're just abandonded and the garbage collector clears the object on the next pass. Am I missing something?

@kevingorski
Copy link

The params variable gets properties set before a match is determined while in a for loop, but it's defined outside of the for loop. So if path A is "#A/:id" and path B is "#B/:foo(/:id)" and path A is checked first, rejected, then B matched for path "#B/value", params["id"] will be "value" and so will params["foo"], where I would expect only "foo" to have been part of the final set of parameters.

@angelyordanov
Copy link

Hey @mtrpcic, I have stumbled on the same bug and fixid it the same as @vincenttheeten . You can find a test case reproducing it in my commit d7d61ff. I can prepare a proper pull requst with all test files fixed if you like.

@tkw722
Copy link

tkw722 commented Mar 11, 2013

I just submitted another pull request for what looks like to be the same issue, mine is #56. The gist of it is that if Path runs through several possible routes that don't match the specified route before it finally does find the correct route. In my case it was due to the order in which I had defined my routes coupled with having mapped them in reverse using a while(i--). Due to this Path was iterating through my route definition list in reverse order, which I didn't at first think about. The problem is that all params that Path comes across with each route it attempts to match are accumulated in the params object. When it finally finds the correct route it passes the whole params object to the correct route. This params object includes any params that route expects plus any that Path came across while trying to find that route. So for example:

var myRegisteredRoutes = [
    ['myRoute/:myParam', myHandler]
    ['myOtherRoute/:myOtherParam', myOtherHandler]
];

Let's suppose the route we're triggering is 'myOtherRoute/something', if we iterate through these in order we'll find that the params object passed to myOtherHandler is:

{
    myParam: 'something',
    myOtherParam: 'something'
}

Obviously the problem is that myOtherHandler isn't expecting the myParam parameter. So to prevent this in the match method just after line 61 I simply added the line:

params = {};

to reset the params object with each attempted route match. This way whatever route that is finally matched will only get the params it expects.

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.

5 participants