no doubled history states #656

Merged
merged 2 commits into from Jan 23, 2014

Conversation

Projects
None yet
5 participants
@stevenvachon
Contributor

stevenvachon commented Jan 8, 2014

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Jan 8, 2014

Contributor

Test?

Contributor

daffl commented Jan 8, 2014

Test?

@daffl daffl closed this Jan 8, 2014

@daffl daffl reopened this Jan 8, 2014

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 8, 2014

Contributor

I'm not sure how to test this with anything other than what is seen. I had the same difficulty in #587. I tested back/forward and replacing history states; all works fine only now without doubling.

Contributor

stevenvachon commented Jan 8, 2014

I'm not sure how to test this with anything other than what is seen. I had the same difficulty in #587. I tested back/forward and replacing history states; all works fine only now without doubling.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 9, 2014

Contributor

Is it possible to register a few changes in the history, then call .back() and see if the url stays the same?

Contributor

justinbmeyer commented Jan 9, 2014

Is it possible to register a few changes in the history, then call .back() and see if the url stays the same?

@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 12, 2014

Contributor

As I'd stated in #587, good idea; I'll create a test soon. I'm hoping that I can first fix the bug, though. The cause is can.route.attr's "change" event calling pushState too many times.

Solution(s):
Add a newURL != currentURL check in history.pushState
I'm, of course, open to better ideas.

Here is the flow:

// start
can.route.ready > can.route.setState

a.click >> anchorClickFix > history.pushState

// back and forward only
window.popstate >> can.route.setState


// overridden  function
history.pushState > can.route.setState

can.route.setState > can.route.attr

// onRouteDataChange is only called once, as it should be
can.route.attr > can.route.data > onRouteDataChange >> can.route.bindings.pushstate.setURL > history.pushState
Contributor

stevenvachon commented Jan 12, 2014

As I'd stated in #587, good idea; I'll create a test soon. I'm hoping that I can first fix the bug, though. The cause is can.route.attr's "change" event calling pushState too many times.

Solution(s):
Add a newURL != currentURL check in history.pushState
I'm, of course, open to better ideas.

Here is the flow:

// start
can.route.ready > can.route.setState

a.click >> anchorClickFix > history.pushState

// back and forward only
window.popstate >> can.route.setState


// overridden  function
history.pushState > can.route.setState

can.route.setState > can.route.attr

// onRouteDataChange is only called once, as it should be
can.route.attr > can.route.data > onRouteDataChange >> can.route.bindings.pushstate.setURL > history.pushState
@stevenvachon

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Jan 23, 2014

Contributor

patch-3 was merged into this (patch-4)… patch-3 may conflict with #699
just a heads up to save any time

Contributor

stevenvachon commented Jan 23, 2014

patch-3 was merged into this (patch-4)… patch-3 may conflict with #699
just a heads up to save any time

@ghost ghost assigned justinbmeyer, andykant and zkat Jan 23, 2014

zkat pushed a commit that referenced this pull request Jan 23, 2014

@zkat zkat merged commit 6425d5d into canjs:master Jan 23, 2014

1 check passed

default The Travis CI build passed
Details

@stevenvachon stevenvachon deleted the stevenvachon:patch-4 branch Jan 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment