-
Notifications
You must be signed in to change notification settings - Fork 3k
Dynamic controller selection and replace browser previous history entry when navigating #389
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
Conversation
Conflicts: build/angular-ui-states.js build/angular-ui-states.min.js build/doc/$templateFactory.html build/doc/$urlMatcherFactory.html build/doc/UrlMatcher.html build/doc/global.html build/doc/index.html
Oh, also, I just realized the name is wrong. Should be |
@nateabele I rebuilt my fork due to my mistake of rebasing a pushed branch (which is why this pull request also shows a bunch of commits to be merged incorrectly) and now you can find the branch with the dynamic controller change + failing unit test here https://github.com/adrianhara/ui-router/tree/DynamicController I also renamed the function from It would be great if you'd have some feedback as to why the controller isn't actually instantiated during the tests (even though in a normal app it works fine). I debugged it some, but got lost in promises inside promises (Inception style). Thanks! |
I sincerely hope I'm commenting in the right place. I see that the code is expecting a function that returns a controller's name dynamically. When I commented on it being useful to me I was forgetting that. What I'd like controllerProvider to do is expect (and handle) an injectable function that returns the controller function itself. @timkindberg -- I think this is why I originally thought of these as separate issues and then promptly forgot about this one. |
May I ask why you need that? As it is now:
|
I had actually made a separate request for comments because I forgot that you had this open issue -- I guess you didn't see it. I see value in having a state property that would allow a controller to be provided by a function.
Assuming the app had a service named appControllers and that it had one defined on its myState property, this would cause that controller to be applied to the state. This has advantages in allowing apps to define (and have parsed and persistent) controllers without worrying about their names or naming collisions, to organize their controllers in a manner that makes sense for the app, and would work very nicely in the context of the upcoming Decorator API (#399). I think the right thing to do would be to have controllerProvider determine whether what it receives from the function is a string or a function. If it's a string, it should be interpreted as a controller's name, and if it is a function it should be interpreted as the controller itself. This is how the controller property works, as well. |
Well, given that |
controller needs to be the function itself. controllerProvider would be a function that returns a function (which is a controller) or a function that returns a string (which is the name of a controller)... analogous to controller but one level deeper. |
Are we saying the same thing? |
Yeah, I was gonna say, I'm lost as to how that differs from what I just said. |
Wanna hear something crazy? Since in detour I need to keep everything as json, I came up with an additional abstraction for these things where ui-router wants to see javascript (such as the xxxProvider properties): A second option -- an object (instead of a function)
is equivalent to
Now I realize that this doesn't provide for the world of possibilities that accepting a function does... but assuming you (the app developer) are in charge of where you park your services and the controllers/templates/resolved values/onEnter/onExit functions that they're providing, it's not too much trouble to put them in properties or functions hanging off of a service. And if you do so, your state definition is cleaner (and can travel in JSON). Just a thought. I can understand if a JSON-compatible representation of the javascript needed for ui-router is considered out of scope -- the decorator feature will allow apps to do the translation should they want/need to with ui-router. |
I think there's an issue in the test -- correct me if I'm wrong: in adrianhara@647cfd2#L1R246 :
Isn't this testing the test value dynamicControllerName rather than some attribute of the stateProvider/state service? |
I may be missing something, but this doesn't appear to be supporting injection -- it appears to be manually passing $stateParams alone. |
@nateabele and @stu-salsbury you are not saying the same thing. Currently we accept:
@stu-salsbury and others are asking for:
Now the question is, in the current implementation, when using a function for controller, instead of the function serving as the controller, can it already instead return a string (controller name) or function (controller function)? If so, we don't need controllerProvider at all. |
@timkindberg -- thanks for helping clarify -- yes, if you assign a function to the controller property it is assumed to be the controller -- not a function that returns a controller. |
Yeah. A decorator implemented in a plugin is definitely the route to go with this. That way you can handle all the individual properties that require functions in a consistent and uniform way. However, there's probably a big enough use case for this that promoting it as a 3rd-party resource in official UI-Router docs would be totally appropriate.
@timkindberg No, we are, don't worry. The unit tests will prove all. :-) |
@stu-salsbury you're right, in my implementation the $stateParams are simply passed to that function, but I guess we could change that to inject stuff? I actually only needed to resolve the controller based on the $stateParams, but I can see the value in being able to inject other things into that function. |
@adrianhara See |
Much thanks for all the great work in this project and issue. Any ballpark idea on when this feature might land into master? I don't have a sense of if we're days or weeks out. Knowing would help me plan for something I'm working on. |
@jasonkuhrt No idea. Really depends on @adrianhara fixing up the commits and adding test coverage. You or someone else are welcome to help or recruit help, but it isn't really a priority for any active maintainers right now. |
@adrianhara if you're stalled let me know and I can take a stab at it. |
Sure guys, I appreciate any help you can provide. Basically the feature works, we are using it in our current project on a daily basis. The only thing is, I can't get the unit test working because in the test the code which instantiates the controller (and also gets the name or provider of the controller) never runs. At the moment I don't have the time to investigate it further, earliest would be end of next week. You can find the failing unit test in my fork: https://github.com/adrianhara/ui-router/tree/DynamicController If you have any ideas they would be greatly appreciated! p.s.: commits are already fixed in my fork (i redid the fork/branch) so if we can get the unit test working I can submit another pull request with a single commit for this change. |
@nateabele @adrianhara et al great job, cheers. |
Thank you @nateabele for cleaning it up! |
Hi guys,
I've done two changes:
Thanks for considering them!