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

YUI Router pushes state before verifying a matching route #1722

Closed
ericsoco opened this issue Mar 25, 2014 · 7 comments
Closed

YUI Router pushes state before verifying a matching route #1722

ericsoco opened this issue Mar 25, 2014 · 7 comments

Comments

@ericsoco
Copy link

A console.log() just before history.add fires before a console.log() just before a path is matched against a regex.

This means that if a user clicks a link to a path on the current domain (routed by YUI App) that does not match any configured routes, the URL will update but navigation will not actually happen, essentially rendering the link broken.

@rgrove
Copy link
Contributor

rgrove commented Mar 26, 2014

It's been a little while since I was in Router, but changing the URL regardless of whether there are any matching routes is by design; if anything, it seems like the fault here is probably that we don't provide an easy way to handle this situation. Or have I misunderstood you?

@ericf
Copy link
Member

ericf commented Mar 26, 2014

Yeah, so save() or replace() will accept any URL and gladly update the address bar to that URL, which will trigger is dispatch and there might be no missing routes.

In the case of Pjax (which is what I think you're seeing here with Y.App and links), when a link is clicked it checks if any routes are matched, if there is one it will call save(), and the URL will update, etc.

A common problem with this is that you can setup a route handler for '*' which will match any URL, i.e. app.hasRoute('/any/random/jazz/'); // => true. This is one possible/common thing that can lead to the behavior you describe.

Here's a basic example app showing how you can get around this by overriding Router's hasRoute() method: https://github.com/ericf/pjax-blacklist/blob/master/public/app.js

The other thing I can think of is using Router's params feature. These are very useful for more complex routes where normalization or validation should happen on the route params. Currently the hasRoute() method does not check all of the params match the given URL. This is something we can/should change.

My guess is that one of these two things is causing the issue you see.

@ericsoco
Copy link
Author

Hi guys, thanks for the responses. We're using Router's param feature. Sorry if that wasn't clear; that's what I was dissecting in this ticket.

We had a fallthrough '*' route, but it was causing another bug (@ericf, issue 1740 in our repo). In that case, we were sending fallthrough routes to our server to ensure our edge services could apply serverside rules to the URL and route it away from our app. In that case, YUI Router was pushing state before calling window.location to send to the server, effectively breaking the back button.

Re: hasRoute()...looking through the code it looks like Pjax calls this, correct? And could avoid pushing state if hasRoute() was more strict / checked all params? If I'm understanding it correctly, a stricter hasRoute() should fix both this and #1723.

FWIW, #1723 isn't really a dupe, but perhaps is moot with the above change to hasRoute().

@ericf
Copy link
Member

ericf commented Mar 26, 2014

FWIW, #1723 isn't really a dupe, but perhaps is moot with the above change to hasRoute().

Yeah, exactly. I'll get a PR up for a change to match() and hasRoute() to respect the router params feature. That way we can all review in and it will make it's way into the next YUI release, and you guys can copy the patch into your code base locally to fix the problem.

@ericsoco
Copy link
Author

@ericf great. Would you mind pinging me on my corp account when that's up? Email or IM.

ericf added a commit to ericf/yui3 that referenced this issue Mar 27, 2014
…dlers

The addition of router params in YUI 3.12.0 provided a way for extra validation of path params during a router's dispatching process. But at the time the router params feature was added, the `hasRouter()` method was not updated to follow suit.

With these changes a router's `hasRoute()` method will process param values the same way its dispatching process does. This makes it an effective way to test whether a router *will* dispatch a given URL.

**Note:** The `match()` method is left unchanged. Both the dispatching process and the `hasRoute()` method first call `match()` to get a collection of possible routes, from which they further process by checking whether each named param handler accepts or rejects a given param value.

Closes yui#1722
Relates yui#1723
@ericsoco
Copy link
Author

Deployed, looking good! Thanks again @ericf.

@ericf
Copy link
Member

ericf commented Mar 28, 2014

Cool! I'll close this out…

@ericf ericf closed this as completed Mar 28, 2014
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

No branches or pull requests

3 participants