search

AmpersandJS / ampersand-router

Clientside router with fallbacks for browsers that don't support pushState. Mostly lifted from Backbone.js.
MIT License
70 stars 16 forks source link

remove erroneous urlChanged check #43

Closed wraithgar closed 9 years ago

wraithgar commented 9 years ago

Currently ampersand-router does not work because this.fragement is changed and then urlChanged is called, which will subsequently always return false.

Also the test for this appeared to be broken, and passing only because of this bug.

fyockm commented 9 years ago

:+1: works for me now

lukekarrys commented 9 years ago

Running this locally, I see that the test does need to be changed but I'm trying to figure out why. Previously the code in this test worked as it was https://github.com/AmpersandJS/ampersand-router/blob/d86e0bcb2180bb0b8fde098d2f68d9018a3fd0b7/test/index.js#L293-L302. Is it worth exploring as to why the original test no longer passes?

lukekarrys commented 9 years ago

It seems to me like that test is trying to handle a case when pushState is false and the current url's path is described by the #route and that calling navigate('route') shouldn't trigger a route change, but it does currently.

I'm +1 on the erroneous urChanged check being removed, but -1 on the test being modified. I'm working on debugging the test right now and will see what I can find.

wraithgar commented 9 years ago

The original test passed because urlChanged was always going to return false. You could have navigated anywhere and the test would have still passed.

wraithgar commented 9 years ago

And yes I wasn't 100% on the test, push up whatever you find if you can make it closer to the original.

lukekarrys commented 9 years ago

The urlChanged check was added pretty recently in 701580944c2860906d769d55911bbd225d659dfc. I checked out the commit before that (d86e0bcb2180bb0b8fde098d2f68d9018a3fd0b7) and stepped through the test in question and the reason loadUrl is never called is because the trigger option defaults to false, so the test is not testing for what it says (loadUrl is not called for identical routes.) but instead it ends up testing that loadUrl is never called because trigger is false.

So I'm a +1 on this now, because its obvious that the extra urlChanged() check breaks stuff and the existing test is now actually testing what it should.

wraithgar commented 9 years ago

published as v3.0.2