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

can.route.ready called multiple times #473

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ccummings
Contributor

ccummings commented Sep 11, 2013

can.route.ready is being called twice (or more if the user calls it themselves) which calls setState and binds to the hashChange event each time.

can.route.ready is the handler for the ready event (L503) and is then called on L508. This results in 2 bindings to the hashChange event (more if user calls can.route.ready themselves).

The other issue is that any modifications to the route before the ready event are wiped out by one of the other setState calls.

I've added a flag that only allows ready to be called once.

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Sep 11, 2013

Contributor

I realized my first attempt at a fix was pretty ham fisted so I've changed it to have a lighter touch.

If you are using jQuery, it will fire a ready event so we don't need to also check the readyState of the DOM. This new fix allows a user to call can.route.ready as much as they want, but it will only be called once on page load.

Contributor

ccummings commented Sep 11, 2013

I realized my first attempt at a fix was pretty ham fisted so I've changed it to have a lighter touch.

If you are using jQuery, it will fire a ready event so we don't need to also check the readyState of the DOM. This new fix allows a user to call can.route.ready as much as they want, but it will only be called once on page load.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 12, 2013

Contributor

We can get that into 1.1.8 I do however still think that we never should call can.route.ready automatically for 1.2. There is so much confusion around when to use it and when not that it will be a lot easier to explain one way of doing things (if you want to start routing call can.route.read(true)).

Contributor

daffl commented Sep 12, 2013

We can get that into 1.1.8 I do however still think that we never should call can.route.ready automatically for 1.2. There is so much confusion around when to use it and when not that it will be a lot easier to explain one way of doing things (if you want to start routing call can.route.read(true)).

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Sep 12, 2013

Contributor

+1
Absolutely agree that ready should have to be called explicitly. I'll create a new issue for that.

Contributor

ccummings commented Sep 12, 2013

+1
Absolutely agree that ready should have to be called explicitly. I'll create a new issue for that.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 20, 2013

Contributor

I don't think there is going to be a 1.8. So this is probably not worth bringing in.

Contributor

justinbmeyer commented Sep 20, 2013

I don't think there is going to be a 1.8. So this is probably not worth bringing in.

@ccummings ccummings deleted the route-ready-fix branch Apr 11, 2014

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