[3.18] `navigate()` falls through to window.location on no matching route. #1874

ericsoco · 2014-06-16T23:11:37Z

Per previous conversations with @ericf.
If this looks like an acceptable solution, I can add tests.

yahoocla · 2014-06-16T23:11:38Z

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

pdokas · 2014-06-16T23:14:08Z

src/pjax/js/pjax-base.js

+        }
+        if (handleFallThrough) {
+            console.log("FALL THROUGH");
+            win.location = url;


We probably want to ensure we're staying on this domain before assigning to window.location.

@pdokas that happens on the line above! Or...wait. Does Y.Error throw an error or just log it?

yahoocla · 2014-06-16T23:25:34Z

CLA is valid!

jeremyruppel · 2014-06-16T23:28:40Z

src/pjax/js/pjax-base.js

    }
 };

-Y.PjaxBase = PjaxBase;
+Y.PjaxBase = PjaxBase;


Newline police! 🚓

rgrove · 2014-06-17T00:38:00Z

Does this even need to be an option? It seems like sensible default behavior (in fact, if you had asked me, I would have sworn it was already the default).

If it does end up remaining an option, then handleFallThroughNavigation seems like an odd name for the attribute, especially since "handle" is a useless verb (all code "handles" things). I don't think the verbosity really communicates anything of value.

Possible alternate names:

allowFallThrough
fallThrough
navigateOnUnmatchedUrl (if you're not into the whole brevity thing)

ericsoco · 2014-06-17T04:39:01Z

@rgrove as I mentioned in the inline comment I just left, I don't think it needs to be an option. (We thought it would be the default as well! Lo and behold it is not.)

The attribute name is *handle*FallThroughNavigation because it implies that the alternative is to not handle it -- let it fall through and silently do nothing (which screwed us and our users, so, agreed -- not a good default). allowFallThrough works for me, though.

ericf · 2014-06-17T13:37:25Z

Does this even need to be an option?

I agree, no need for an option. I think this change will, in the end, reduce the complexity overall.

rgrove · 2014-06-17T18:06:44Z

To clarify: by "does this even need to be an option?" I mean that it doesn't seem like either the navigate() option or the attribute are necessary.

If navigate() gets called with a URL that doesn't match a registered route, it seems totally sensible to do a full page navigation to that URL. If that's not what the caller wanted, they shouldn't have called navigate(), right?

ericsoco · 2014-06-17T19:39:56Z

@rgrove we expected that to be the default behavior and were surprised to find that it was not. That said, if there are any YUI consumers out there relying on the current behavior, an attribute would at least give them the option to disable this change in the default.

Just pushed an update based on the conversation so far.

ericsoco · 2014-06-19T01:05:41Z

Any thoughts on the latest?

rgrove · 2014-06-19T16:40:21Z

👍

ericsoco · 2014-06-26T23:35:48Z

Sorry, been meaning to get back to this (write tests), but end of quarter happened. Still on my radar...somewhere....

ericsoco · 2014-08-06T18:37:12Z

Getting back to this after a long hiatus...

There's a failing test here that I'm not sure how to handle:
'navigate event should not fire for a hash URL that resolves to the current page'

This test relies on the false return from _navigate() to cause navigate() to fall through to its own return false. However, the discussion above led me to add a return true to the allowFallThrough condition; seems like this test is no longer useful as-is.

Specifically, a hash URL that resolves to the current page should cause _navigate() to return false, but navigate() should still return true. Do you agree?

ericsoco · 2014-08-06T18:54:02Z

Also, how do I specify the router config from pjax-test.js / the test environment in general? I can't figure out how to test a non-matching route (which is what this PR is all about). If it's relatively simple for the YUI team to run tests on this PR, that would be great -- I've burned 2 hours this morning getting this far and I'm still at a wall.

triptych · 2014-08-06T21:28:45Z

Adding @tripp @ericf and @jlecomte for /CC

ericsoco · 2014-08-08T16:11:28Z

Hello again, @triptych @tripp @ericf @jlecomte @rgrove.
Would love to close this out before 3.18; would love to close it out in general. Any word on help with these tests?
/cc @pdokas

tripp · 2014-08-08T16:45:11Z

@ericsoco
I'll be able to look into this on Monday.

ericsoco · 2014-08-12T19:04:38Z

Hi @tripp, any news?

tripp · 2014-08-12T19:59:26Z

@ericsoco No. I only pulled in your branch this morning. My apologies. I'll post back later tonight.

tripp · 2014-08-13T05:18:18Z

Pull request #1937 updates the tests for your fix.
I'm thinking those test updates should work. I can merge these changes in tomorrow.

tripp · 2014-08-14T02:56:35Z

@ericsoco
I didn't get to this today. I'll be able to do it tomorrow. Thanks.

tripp · 2014-08-14T16:13:25Z

I've cherry-picked your commits into dev-master.

ericsoco · 2014-08-14T19:52:48Z

w00t! We'll look for em in 3.18. Thanks for knocking this out, @tripp.

pdokas · 2014-08-14T19:53:33Z

🎉

okuryu · 2014-08-18T12:20:19Z

I updated the change log at e2cd21b just in case.

navigate() falls through to window.location on no matching route.

8621a85

pdokas reviewed Jun 16, 2014
View reviewed changes

Don't handleFallThrough if different origin.

1e2bb59

jeremyruppel reviewed Jun 16, 2014
View reviewed changes

src/pjax/js/pjax-base.js

}

};

Y.PjaxBase = PjaxBase;

Y.PjaxBase = PjaxBase;

Copy link

jeremyruppel Jun 16, 2014

Choose a reason for hiding this comment

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

Newline police! 🚓

Remove option, rename attrib, remove console.log, use @SInCE@

639ec2a

Remove no-longer-necessary local var.

5f25d3d

Merge branch 'master' into fallthru-navigation

fef35fb

triptych changed the title ~~navigate() falls through to window.location on no matching route.~~ [3.18] navigate() falls through to window.location on no matching route. Aug 6, 2014

tripp mentioned this pull request Aug 13, 2014

Adds test updates to #1874 #1937

Closed

tripp closed this Aug 14, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.18] `navigate()` falls through to window.location on no matching route. #1874

[3.18] `navigate()` falls through to window.location on no matching route. #1874

ericsoco commented Jun 16, 2014

yahoocla commented Jun 16, 2014

pdokas Jun 16, 2014

ericsoco Jun 16, 2014

yahoocla commented Jun 16, 2014

jeremyruppel Jun 16, 2014

rgrove commented Jun 17, 2014

ericsoco commented Jun 17, 2014

ericf commented Jun 17, 2014

rgrove commented Jun 17, 2014

ericsoco commented Jun 17, 2014

ericsoco commented Jun 19, 2014

rgrove commented Jun 19, 2014

ericsoco commented Jun 26, 2014

ericsoco commented Aug 6, 2014

ericsoco commented Aug 6, 2014

triptych commented Aug 6, 2014

ericsoco commented Aug 8, 2014

tripp commented Aug 8, 2014

ericsoco commented Aug 12, 2014

tripp commented Aug 12, 2014

tripp commented Aug 13, 2014

tripp commented Aug 14, 2014

tripp commented Aug 14, 2014

ericsoco commented Aug 14, 2014

pdokas commented Aug 14, 2014

okuryu commented Aug 18, 2014

[3.18] navigate() falls through to window.location on no matching route. #1874

[3.18] navigate() falls through to window.location on no matching route. #1874

Conversation

ericsoco commented Jun 16, 2014

yahoocla commented Jun 16, 2014

pdokas Jun 16, 2014

Choose a reason for hiding this comment

ericsoco Jun 16, 2014

Choose a reason for hiding this comment

yahoocla commented Jun 16, 2014

jeremyruppel Jun 16, 2014

Choose a reason for hiding this comment

rgrove commented Jun 17, 2014

ericsoco commented Jun 17, 2014

ericf commented Jun 17, 2014

rgrove commented Jun 17, 2014

ericsoco commented Jun 17, 2014

ericsoco commented Jun 19, 2014

rgrove commented Jun 19, 2014

ericsoco commented Jun 26, 2014

ericsoco commented Aug 6, 2014

ericsoco commented Aug 6, 2014

triptych commented Aug 6, 2014

ericsoco commented Aug 8, 2014

tripp commented Aug 8, 2014

ericsoco commented Aug 12, 2014

tripp commented Aug 12, 2014

tripp commented Aug 13, 2014

tripp commented Aug 14, 2014

tripp commented Aug 14, 2014

ericsoco commented Aug 14, 2014

pdokas commented Aug 14, 2014

okuryu commented Aug 18, 2014

[3.18] `navigate()` falls through to window.location on no matching route. #1874

[3.18] `navigate()` falls through to window.location on no matching route. #1874