Default can.route.ready to false #298

Closed
daffl opened this Issue Mar 5, 2013 · 16 comments

Comments

Projects
None yet
5 participants
@daffl
Contributor

daffl commented Mar 5, 2013

For 1.2 we should default can.route.ready to always be false and let the user initialize routing in any case.

When to use can.route.ready and when not seems to cause quite a bit of confusion and in almost every case you need it anyway. Plus we can get rid of the rather hacky document ready detection (which had to be added because the document ready handler in libraries other than jQuery doesn't get called if the document is ready by the time it is attached).

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 5, 2013

Contributor

I worry about basic apps. Those using can/control/route.

On Mar 5, 2013, at 10:08 AM, David Luecke notifications@github.com wrote:

For 1.2 we should default can.route.ready to always be false and let the user initialize routing in any case.

When to use can.route.ready and when not seems to cause quite a bit of confusion and in almost every case you need it anyway. Plus we can get rid of the rather hacky document ready detection (which had to be added because the document ready handler in libraries other than jQuery doesn't get called if the document is ready by the time it is attached).


Reply to this email directly or view it on GitHub.

Contributor

justinbmeyer commented Mar 5, 2013

I worry about basic apps. Those using can/control/route.

On Mar 5, 2013, at 10:08 AM, David Luecke notifications@github.com wrote:

For 1.2 we should default can.route.ready to always be false and let the user initialize routing in any case.

When to use can.route.ready and when not seems to cause quite a bit of confusion and in almost every case you need it anyway. Plus we can get rid of the rather hacky document ready detection (which had to be added because the document ready handler in libraries other than jQuery doesn't get called if the document is ready by the time it is attached).


Reply to this email directly or view it on GitHub.

@jeffreytgilbert

This comment has been minimized.

Show comment
Hide comment
@jeffreytgilbert

jeffreytgilbert Mar 20, 2013

I'm having issues with this. CanJS is rewriting a route and escaping characters in it before i can stop it from running and doing so. It's extremely confusing because I haven't even told CanJS about any routes. In fact, removing all routing declarations and just loading CanJS on a page will alter my routes.

Reasons it shouldn't be run by default:

  • No routing may even be loaded, so it's doing things it's not asked to be doing, which is contrary to expectations
  • Third party routing libraries will be interfered with, thereby breaking "library-er" conventions of CanJS
  • Deferring the route handling to a point where the asynchronous modules have a chance to load and respond in their own order just makes more sense
  • Adding a ready call at the end of a route declaration is sufficiently easy to understand for basic app builders

I'm having issues with this. CanJS is rewriting a route and escaping characters in it before i can stop it from running and doing so. It's extremely confusing because I haven't even told CanJS about any routes. In fact, removing all routing declarations and just loading CanJS on a page will alter my routes.

Reasons it shouldn't be run by default:

  • No routing may even be loaded, so it's doing things it's not asked to be doing, which is contrary to expectations
  • Third party routing libraries will be interfered with, thereby breaking "library-er" conventions of CanJS
  • Deferring the route handling to a point where the asynchronous modules have a chance to load and respond in their own order just makes more sense
  • Adding a ready call at the end of a route declaration is sufficiently easy to understand for basic app builders
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 21, 2013

Contributor

You can make a download without can/route. That should solve the issues you're having.

Contributor

justinbmeyer commented Mar 21, 2013

You can make a download without can/route. That should solve the issues you're having.

@jeffreytgilbert

This comment has been minimized.

Show comment
Hide comment
@jeffreytgilbert

jeffreytgilbert Mar 21, 2013

If I don't use the complicated routes with parameters, it doesn't happen, but then i also can't use those features. I'm using the AMD / RequireJS version, not the downloaded bundle.

If I don't use the complicated routes with parameters, it doesn't happen, but then i also can't use those features. I'm using the AMD / RequireJS version, not the downloaded bundle.

@jeffreytgilbert

This comment has been minimized.

Show comment
Hide comment
@jeffreytgilbert

jeffreytgilbert Mar 21, 2013

BTW, this bug can be seen in the TODO MVC example as well as my application. Here's a url to showcase what is happening.

http://addyosmani.github.com/todomvc/labs/dependency-examples/canjs_require/#!active/andthis&butthis=thisval

Watch the / after active to see what's happening. It gets encoded, and if it were a route, it would no longer match the route definition. You can actually see this by adding a number of characters like so:

http://addyosmani.github.com/todomvc/labs/dependency-examples/canjs_require/#!active;andthis=this&butthis=thisval

The semicolon will be encoded as well as the following = sign.

BTW, this bug can be seen in the TODO MVC example as well as my application. Here's a url to showcase what is happening.

http://addyosmani.github.com/todomvc/labs/dependency-examples/canjs_require/#!active/andthis&butthis=thisval

Watch the / after active to see what's happening. It gets encoded, and if it were a route, it would no longer match the route definition. You can actually see this by adding a number of characters like so:

http://addyosmani.github.com/todomvc/labs/dependency-examples/canjs_require/#!active;andthis=this&butthis=thisval

The semicolon will be encoded as well as the following = sign.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 21, 2013

Contributor

If I don't use the complicated routes with parameters, it doesn't happen, but then i also can't use those features. I'm using the AMD / RequireJS version, not the downloaded bundle.

What do you mean?

BTW, this bug ...

This issue is about can.route.ready being set to false. I'm not sure exactly what you are talking about. However, if you think there's another bug, can you create another issue? Also, can you carefully describe the bug? Ideally someone should be able to read the comment and know immediately what you are talking about. Thanks.

EDIT:

After spending some time trying to understand what you meant, I'm not even sure why #!active/andthis&butthis=thisval would be a route? Nothing in the app set it that way. There's only one route:

https://github.com/addyosmani/todomvc/blob/gh-pages/architecture-examples/canjs/js/app.js#L6

can.route(':filter');

With just this one route, I would expect a hash like #!foo/bar to get converted to #!foo%2Fbar so can.route.attr() would return:

{filter: "foo/bar", route: ":filter"}

I think you might be misunderstanding the idea behind can.route. can.route isn't as much about matching a hash as it is keeping state.

say, for example there were two routes:

can.route(':filter');
can.route(':filter/:something');

And the user set can.route w/ .attr() like:

can.route.attr("filter","foo/bar")

The hash would have to be set to #!foo%2Fbar. If it was set to #!foo/bar, the data would be:

{filter: "foo", something: "bar", route: ":filter/:something"}

Basically, can.route does whatever it has to so the has represents one unique state.

Contributor

justinbmeyer commented Mar 21, 2013

If I don't use the complicated routes with parameters, it doesn't happen, but then i also can't use those features. I'm using the AMD / RequireJS version, not the downloaded bundle.

What do you mean?

BTW, this bug ...

This issue is about can.route.ready being set to false. I'm not sure exactly what you are talking about. However, if you think there's another bug, can you create another issue? Also, can you carefully describe the bug? Ideally someone should be able to read the comment and know immediately what you are talking about. Thanks.

EDIT:

After spending some time trying to understand what you meant, I'm not even sure why #!active/andthis&butthis=thisval would be a route? Nothing in the app set it that way. There's only one route:

https://github.com/addyosmani/todomvc/blob/gh-pages/architecture-examples/canjs/js/app.js#L6

can.route(':filter');

With just this one route, I would expect a hash like #!foo/bar to get converted to #!foo%2Fbar so can.route.attr() would return:

{filter: "foo/bar", route: ":filter"}

I think you might be misunderstanding the idea behind can.route. can.route isn't as much about matching a hash as it is keeping state.

say, for example there were two routes:

can.route(':filter');
can.route(':filter/:something');

And the user set can.route w/ .attr() like:

can.route.attr("filter","foo/bar")

The hash would have to be set to #!foo%2Fbar. If it was set to #!foo/bar, the data would be:

{filter: "foo", something: "bar", route: ":filter/:something"}

Basically, can.route does whatever it has to so the has represents one unique state.

@jeffreytgilbert

This comment has been minimized.

Show comment
Hide comment
@jeffreytgilbert

jeffreytgilbert Mar 21, 2013

I've added another issue here per your request:

#330

I've added another issue here per your request:

#330

@jeffreytgilbert

This comment has been minimized.

Show comment
Hide comment
@jeffreytgilbert

jeffreytgilbert Mar 21, 2013

You're absolutely right about that not being a route that the TODO MVC application is expecting. It wouldn't matter even if it were. CanJS shouldn't be manipulating those hashes. If it were actually expecting those routes, it wouldn't change the behavior of the page. The encoding would still happen and the routes would still break.

You're absolutely right about that not being a route that the TODO MVC application is expecting. It wouldn't matter even if it were. CanJS shouldn't be manipulating those hashes. If it were actually expecting those routes, it wouldn't change the behavior of the page. The encoding would still happen and the routes would still break.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 21, 2013

Contributor

If you are using the amd version, don't require 'can' directly, create a custom 'mycan' that requires only the modules you want.

Also, you might be able to simply call

can.route.ready(false)

and never call .ready(true). As you are using requirejs, to use the plugin that prevent's jQuery's ready event from firing until all scripts have loaded.

Contributor

justinbmeyer commented Mar 21, 2013

If you are using the amd version, don't require 'can' directly, create a custom 'mycan' that requires only the modules you want.

Also, you might be able to simply call

can.route.ready(false)

and never call .ready(true). As you are using requirejs, to use the plugin that prevent's jQuery's ready event from firing until all scripts have loaded.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 21, 2013

Contributor

If it were actually expecting those routes .... the encoding would still happen and the routes would still break.

That's not right. Check out the example in #330.

I think the best solution for @jeffreytgilbert is simply not to load can.route if you aren't using it. Don't waste time loading code you aren't using.

If we should call force a call to can.route.ready() when someone has included the plugin (indicating desire of its features) is another matter ...

Contributor

justinbmeyer commented Mar 21, 2013

If it were actually expecting those routes .... the encoding would still happen and the routes would still break.

That's not right. Check out the example in #330.

I think the best solution for @jeffreytgilbert is simply not to load can.route if you aren't using it. Don't waste time loading code you aren't using.

If we should call force a call to can.route.ready() when someone has included the plugin (indicating desire of its features) is another matter ...

@jeffreytgilbert

This comment has been minimized.

Show comment
Hide comment
@jeffreytgilbert

jeffreytgilbert Mar 21, 2013

I'm currently using the routing for simple routes. Complex ones i'll have another pass at a few months down the road once i've finished everything mission critical on this project.

On Mar 21, 2013, at 3:08 PM, Justin Meyer notifications@github.com wrote:

If it were actually expecting those routes .... the encoding would still happen and the routes would still break.

That's not right. Check out the example in #330.

I think the best solution for @jeffreytgilbert is simply not to load can.route if you aren't using it. Don't waste time loading code you aren't using.

If we should call force a call to can.route.ready() when someone has included the plugin (indicating desire of its features) is another matter ...


Reply to this email directly or view it on GitHub.

I'm currently using the routing for simple routes. Complex ones i'll have another pass at a few months down the road once i've finished everything mission critical on this project.

On Mar 21, 2013, at 3:08 PM, Justin Meyer notifications@github.com wrote:

If it were actually expecting those routes .... the encoding would still happen and the routes would still break.

That's not right. Check out the example in #330.

I think the best solution for @jeffreytgilbert is simply not to load can.route if you aren't using it. Don't waste time loading code you aren't using.

If we should call force a call to can.route.ready() when someone has included the plugin (indicating desire of its features) is another matter ...


Reply to this email directly or view it on GitHub.

@amcdnl

This comment has been minimized.

Show comment
Hide comment
@amcdnl

amcdnl Apr 8, 2013

Contributor

+1 I think we should start this off as false too.

Its a pretty common thing to need to do, shouldn't have to have the users doing it.

Contributor

amcdnl commented Apr 8, 2013

+1 I think we should start this off as false too.

Its a pretty common thing to need to do, shouldn't have to have the users doing it.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 12, 2013

Contributor

Brought here from #475. Again, my big hesitation is that we will have to tell people they MUST call can.route.ready() to even use routing in the first place. The getting started guide will have can.route.ready().

Contributor

justinbmeyer commented Sep 12, 2013

Brought here from #475. Again, my big hesitation is that we will have to tell people they MUST call can.route.ready() to even use routing in the first place. The getting started guide will have can.route.ready().

@ccummings

This comment has been minimized.

Show comment
Hide comment
@ccummings

ccummings Sep 12, 2013

Contributor

The confusion/frustration from the automatic ready is likely related to #473 where ready is called once (before user has a chance to delay) and then again when jQuery's ready event is fired.

Backbone and Ember both require an explicit call to start routing using Backbone.history.start() and Ember.Application.create(...) respectively.

In Ember's case it will start routing on DOMReady, but you can control that using deferReadiness and advanceReadiness. Angular does something similar except it seems much more tricky to delay route initialization.

I'd be fine with automatic route.ready on DOMReady if we added a ready event to all libraries which gives the user a chance at delaying the route initialization.

Contributor

ccummings commented Sep 12, 2013

The confusion/frustration from the automatic ready is likely related to #473 where ready is called once (before user has a chance to delay) and then again when jQuery's ready event is fired.

Backbone and Ember both require an explicit call to start routing using Backbone.history.start() and Ember.Application.create(...) respectively.

In Ember's case it will start routing on DOMReady, but you can control that using deferReadiness and advanceReadiness. Angular does something similar except it seems much more tricky to delay route initialization.

I'd be fine with automatic route.ready on DOMReady if we added a ready event to all libraries which gives the user a chance at delaying the route initialization.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Sep 12, 2013

Contributor

I found it way harder having to explain (and document) when to use can.route.ready and why it works in one case but doesn't in another. This comes up in IRC and Stackoverflow all the time and it just doesn't seem to work how many people (including myself) expect it to.

Plus personally when I do things like:

var Router = can.Control.extend({
    ':type route': function() {
        // do something
    }
});

new Router();

I always want my route to fire even when the page is refreshed and the router is initialized and for that there is no way around having to call can.route.ready (twice).

Contributor

daffl commented Sep 12, 2013

I found it way harder having to explain (and document) when to use can.route.ready and why it works in one case but doesn't in another. This comes up in IRC and Stackoverflow all the time and it just doesn't seem to work how many people (including myself) expect it to.

Plus personally when I do things like:

var Router = can.Control.extend({
    ':type route': function() {
        // do something
    }
});

new Router();

I always want my route to fire even when the page is refreshed and the router is initialized and for that there is no way around having to call can.route.ready (twice).

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Sep 20, 2013

Contributor

With 520f1b9, you MUST call can.route.ready. This will be part of 1.2.

Contributor

justinbmeyer commented Sep 20, 2013

With 520f1b9, you MUST call can.route.ready. This will be part of 1.2.

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