refactors, fixes and making it more mootools-ish #1

Merged
merged 9 commits into from May 25, 2012

Projects

None yet

2 participants

@DimitarChristoff

basically, moved everything into events, passed on via the options mixin. this stops the need to Router.implement() to change the prototype before you instantiate it, it also raises the events better and passes on all the this.params as arguments to the events callbacks.

api change reflected in readme and demo.

@xrado
Owner

really like your code improvements!
..except "before" and "after" were designed to run before and after route execution. Now with event based triggering methods are run almost in parallel and loose their function.

what do you think?

@DimitarChristoff

I have done some more work on this, namely - events are now much tidier and they actually work as expected.

in particular, you can now add route-specific pseudo events, like:

onUserView:before or onUserView:after

onBefore and onAfter generics now try to pass on the route identifier as an argument as well.

if a route is defined but no event has been added, onError now fires correctly.

onUndefined fires when a route is requested that has no definition in this.routes

as for your concerns, I am not sure - the events are still blocking - if you put cpu heavy code in the :before events, ui thread will lock until they finish before it actually navigates so I would argue, they fire at the right times. perhaps I am misunderstanding you somehow... what is your use case for before/after anyway

left to do: fix the readme.md to reflect new api changes (like in demo) and refrelct that in the gh-pages - will probably do later on today.

ideally: add tests so that functionality does not break - though I doubt it will change much from here on out.

@xrado
Owner

per route/event before and after is a nice addition

before/after are meant for logic that is the same for some group of routes or maybe all routes. Lets say I have this routes defined
#!/apps/app/:id?
#!/apps

in this case you can put a check in before if(route.match(/^!/apps/)) and do some work that is common for both actions. Now if after method depends on something done in before and route action, after code may fall. Sure this can also happen if route have async logic. If I thing again "after" does not have that sense any more

@DimitarChristoff

i see. yeah that should not be breaking but I have been reliant on order of definition and your example is also indicative of how i figured it should be used - think like .htaccess mod_rewrite rules with a [L] flag - first match exits (due to the break in the route loop).

hence, the more qualified routes need to go first and broader ones last as 'catch-all' rules. there is only one problem with this - routes is an object and as per ECMA spec, there is no requirement for FIFO, i.e. there are no guarantees that as we walk the routes object, the keys will come out in the order they were defined. chrome is a prime example of a browser that breaks FIFO as if your object contains numeric keys, a for (var key in obj) will output these first of all.

I guess we could always go for an array but I think with the format of the route keys, it's safe enough. the object gives instant lookup whereas otherwise we'd have to walk the array to find a match. removing routes can also create sparse arrays, so there is a lot more GC involved. speaking of, we probably should add 2 methods that are accessors for adding routes later. addRoute('pattern', 'identifier', eventCallbackObject) and removeRoute('identifier'), what do you think? they can deal with the adding and deleting of the keys in the routes object as well as do the addEvent / removeEvent. the calback objects as api can be just an object literal to add:

{  
    onIndex: fn,
    'onIndex:before': fn
}

or, it can be:

addRoute: function(obj) {
}

used like so:

app.addRoute({
    route: '#!info',
    id: 'info',
    events: {
        'onInfo': fn,
        'onInfo:before': fn
    }
});

doing this route.match logic is probably the wrong thing to do as it cannot stop the firing of the following events anyway.

i don't see harm in having the after events in case you need to do cleanup or update stuff like breadcrumbs etc dynamically after your app navigates away.

@xrado
Owner

I've totally forgot about object key ordering in chrome, I think chrome also order keys numerically and alphabetically, but now I can't remember the case where ordering is important.

yes addRoute would be a nice addition, syntax seems fine to me

@xrado xrado merged commit 0a603fd into xrado:master May 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment