Skip to content
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

Allow for implicit route-definition in can.Control #549

Closed
wants to merge 2 commits into from

Conversation

simpleTechs
Copy link

This change will allow for way easier organization of multiple controls. Instead of binding every control to /:view/:action, you can now bind specific routes that still are matched through the default /:view/:action/:id route.
This allows very easy control-segmentation (one control only occupies the routes that are specifically defined for it) while also allowing to "extract" the parameters via route.attr('view') and alike.

Quick example:

    can.Control.extend("Router1", {
        "/r1 route" : function () {
            console.log('route updated to /r1')
        },

        "/r1/:id route" : function () {
            console.log('route updated to /r1/:id');
        },

        "/r1/:action/:id route" : function () {
            console.log('route updated to /r1/:action/:id')
        }
    });
    can.Control.extend("Router2", {
        "/r2 route" : function () {
            console.log('route updated to /r2')
        },

        "/r2/:id route" : function () {
            console.log('route updated to /r2/:id');
        },

        "/r2/action1/:id route" : function () {
            console.log('route updated to /r2/action1/:id')
        },

        "/r2/action2/:id route" : function () {
            console.log('route updated to /r2/action2/:id')
        },

        "/r2/action3/:id route" : function () {
            console.log('route updated to /r2/action3/:id')
        }
    });
    can.route('/:view');
    can.route('/:view/:id');
    can.route('/:view/:action/:id');

What do you think, would it make sense to allow for this kind of rules? The patches should not break any existing tests (they don't locally), but I do know this is probably not the most performant way to implement it.

Would you be open to a feature like this anyway?
I was not able to achieve similar behavior using only can.route+can.Map.delegate, it would have been way more complex, so brute-force-trying every possible match seems not too bad to me.

This change will allow for way easier organization of multiple controls. Instead of binding every control to `/:view/:action`, you can now bind specific routes that still are matched through the default `/:view/:action` route.

Quick example:
```javascript
	can.Control.extend("Router1", {
		"/r1 route" : function () {
			ok(true, 'route updated to /r1')
		},

		"/r1/:id route" : function () {
			ok(true, 'route updated to /r1/:id');
		},

		"/r1/:action/:id route" : function () {
			ok(true, 'route updated to /r1/:action/:id')
		}
	});
	can.Control.extend("Router2", {
		"/r2 route" : function () {
			ok(true, 'route updated to /r2')
		},

		"/r2/:id route" : function () {
			ok(true, 'route updated to /r2/:id');
		},

		"/r2/action1/:id route" : function () {
			ok(true, 'route updated to /r2/action1/:id')
		},

		"/r2/action2/:id route" : function () {
			ok(true, 'route updated to /r2/action2/:id')
		},

		"/r2/action3/:id route" : function () {
			ok(false, 'route was not updated to /r2/action3/:id')
		}
	});
	can.route('/:view');
	can.route('/:view/:id');
	can.route('/:view/:action/:id');
```
This test is of course a very quick one, but it shows the basic principle.
@daffl
Copy link
Contributor

daffl commented Nov 15, 2013

This looks pretty good. I always felt there is some weirdness going on in our routing. E.g. with can.route('test/:id') and can.route('other/:id') what happens if I do can.route.attr('id', 1234)?
I think it should actually be can.route.attr('test/:id', { id: 123 }). This looks like a good step in the right direction.

@justinbmeyer
Copy link
Contributor

It should not work like: can.route.attr('test/:id', { id: 123 })

You need to give your routes deterministic state like can.route(":page/:id")

Sent from my iPhone

On Nov 15, 2013, at 10:56 AM, David Luecke notifications@github.com wrote:

This looks pretty good. I always felt there is some weirdness going on in our routing. E.g. with can.route('test/:id') and can.route('other/:id') what happens if I do can.route.attr('id', 1234)?
I think it should actually be can.route.attr('test/:id', { id: 123 }). This looks like a good step in the right direction.


Reply to this email directly or view it on GitHub.

@justinbmeyer
Copy link
Contributor

I'm open to this. After talking about it with @daffl, we need to make matching state easier. The delegate plugin is a (poor) attempt to solve this. This looks like an interesting solution for 2.1.

However, it seems like can.route() is being called for each control "route" method.

if ( !can.route.routes[selector] ) {
                        can.route( selector );
}

Should we turn that off and force people to define the routes explicitly? I'm not sure how it actually works otherwise. For example in:

        "/r1/:id route" : function () {
            console.log('route updated to /r1/:id');
        },

is can.route.attr("view") === r1?

@simpleTechs
Copy link
Author

@daffl Well, I think it should depend on where the user is at when the id changes. So if he was on "test" before, this should fire. I don't think my changes do account for this, though.

@justinbmeyer The implicit definition is actually part of canjs since I have used it, and it allows you to "just define the route" without caring about registering it.
Regarding your 2nd question: in fact, it is. Because the pattern that was matched was /:view/:id. The problem is, that if you define another route as /:whatever/:id, this route might be matched first and of course destroy the whole idea of it.

Another problem, I ran into:
Due to the fact the controls must be initialized in order to receive route-events, this will totally mess up a project with existing controls. I didn't really think of this in the first place, but canjs now thinks every control is active, thus listens for all the events, adds the controls name to the element, and so on. So maybe this may serve as a starting point, but it should totally not be included in the source right now. I'm sorry for this!

@justinbmeyer
Copy link
Contributor

I'm moving this to 3.0 because I think it's a great idea, but would require breaking API changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants