Allow multiple calls to App.Router.map

[wmarbut]

I've found a need to be able to add new routes at runtime (one of my apps allows plugins). Upon looking through docs and API, I couldn't find a way to do this so I went to source and still didn't find anything.

Attached is code that allows you to call App.Router.map multiple times without overwriting the previous behavior.

I've written two tests for it because the change is minor. I'm happy to write additional tests or receive guidance from the team.

[joliss]

Some quick comments: These lines seem mis-indented, and perhaps you should add a comment why there is a .hasOwnProperty('length') check?

[wmarbut]

Oops! It looked perfect in Vim. I can fix that. As for the hasOwnProperty call, its my paranoia in a world without type safety. I like to make sure the this.router_map_callbacks is in fact method compatible with an array.

[joliss]

Do you want to use git rebase -i to squash your follow-up commit into the first one? It's always best to have clean history that tells a nice story, so corrections aren't visible in the history. After using rebase, you can push --force into the branch, and GitHub will pick it up.

[wmarbut]

@joliss Thanks, I just rewound, amended, and force pushed.

[joliss]

Okay, I've added some more comments. I hope you don't mind my nitpicking. It looks almost ready to merge.

[joliss]

FYI, my comments ended up on the commit page at DynamiX-Web-Design@704ad67 o_o

[joliss]

Ah, now you have two commits. Want to squash and repush again?

Other than that, looks good to merge to me! 👍

[wmarbut]

@joliss yeah, noticed the two commits :( I've literally been trying to figure out how to squash them since I pushed the first time. My gitfu is not strong, but I think it's good to go now

Thanks for all your help :)

[wmarbut]

Here is a gist that does this in userland https://gist.github.com/grep-awesome/5406461

[joliss]

Cool. Good to merge from my end. 👍

[stefanpenner]

This also seems good to me, although I have not spent much time in router land. Mr API @tomdale what are your thoughts? @machty your doing some router work now, thoughts?

[machty]

nitpick, but this should probably be indented by 4 spaces.

[wmarbut]

Roger that, indenting.

[machty]

Can you explain how Ember.Router, which holds a reference to a single instance of Router from router.js, can function given that you instantiate two separate router.js routers? It doesn't make sense to make, and I have a hunch that your test case isn't fully exploring this oddity.

[wmarbut]

@machty: when I run this in my head, when I instantiate a new copy of Router and assign it to Ember.Router, the old one just gets gc'd. I don't believe there are any other references to it, so unless I'm missing something subtle, the old instance should just go away. However, in a framework, performance is important and gc is expensive; I've updated this code to make use of the existing Router if it has been defined. FWIW aside from my test cases, I'm using this (the userland edition of it https://gist.github.com/grep-awesome/5406461) in a fairly large production app with good results.
Cheers!

[trek]

This looks good to me. @machty, did @grep-awesome address your concern? Will this need to rebased against #2740 when that is merged?

[machty]

@grep-awesome @trek The async router PR shouldn't affect this. I'm still a bit torn, but perhaps we should just merge this for now and come up with a better solution now.

I understand how this PR works, but it still seems like an avoidable hack. We shouldn't be constructing/destroying/recreating the backbone of every Ember app for an action that should merely augment the router's state, and not just for perf/GC reasons. Not that this is the use case you had an mind for this PR, but this might cause some nasty surprises once someone tries to run a second .map after their app has initialized, which I believe would probably irreparably break the app.

But I understand that's not the use case of this PR. I guess I'm ok with merging it, since the implications are not far-reaching for this PR and if/when we want to support calling .map after app load, we can just rework some of the logic in this PR. (I do think we'll want that at some point as a solution for dynamically loading routes mid-app, though there are other conceptual problems to solve with that.)

[wmarbut]

@machty I'm glad to hear that you are good to merge, however I also agree with your reservations. It is somewhat of a hack, but as you have said, there are some conceptual problems with supporting dynamically loading routes mid-app that are well outside the scope of this request. My goal here was to provide a light touch solution to a need without altering logic at a level that might be foundational to ember's philosophy.

[machty]

grep-awesome could you change your PR so that it saved up mapCallbacks to be executed when the app is initializing? That way it'd preserve all the same behavior and it wouldn't be discarding a Router instance.

[wmarbut]

@machty Unless I'm misunderstanding you, I've already changed the behavior here to not discard the Router instance. We are still regenerating the DSL though, is this what you are asking me to defer?

Apologies for the confusion, I'm in #emberjs if a direct chat resolves this more easily

[machty]

Oh crap, I'm a dummy.

I think this PR is good to merge :)

[stefanpenner]

this does not rebase cleanly

[wmarbut]

@stefanpenner I'll squash, rebase, and push around 2100UTC. Thanks!

[wmarbut]

@stefanpenner I see the conflict and even was able to merge it here: https://gist.github.com/grep-awesome/bffdf7b8b6112cf0aa57, but I have no idea how to make this happen correctly in git for my pull request. Any pointers would be most welcome, otherwise I'll have to dig into git docs and irc tomorrow for help.

[stefanpenner]

@grep-awesome git fetch and a rebase against origin master didn't work? Make sure you are fetching the correct remote repo.

If by tomorrow evening you where unable to resolve this I will gladly doit.