Dynamic controller selection and replace browser previous history entry when navigating

[adrianhara]

Hi guys,

I've done two changes:

• added optional "controllerFactory" property to views: it should be function that'll get called with $stateParams as argument and it can return the name of the controller to use; basically allow dynamically choosing the controller for a view; I submitted this before, there was a discussion and in the end it was closed because of lack of activity, however it would be really great if it could be added as we're using it heavily and it's opt-in, so no harm done theoretically.
• added optional "replacePreviousHistoryEntry" parameter to state navigation options: if set to 'true' (default 'false') then the navigation will replace the previous browser history entry. A use-case for this would be when you have a state for an "add" screen and on "save" on that screen you navigate to a "details" state. However, if then the user goes back in the browser he'll end up on the "add" screen again, which might not be ideal: using this parameter as 'true' will cause the back navigation to actually go to the previous page (i.e. before the "add" screen).

Thanks for considering them!

[nateabele]

Oh, also, I just realized the name is wrong. Should be controllerProvider to match up with the corresponding template key.

[adrianhara]

@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 controllerFactory to controllerProvider as you suggested.

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!

[laurelnaiad]

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.

[adrianhara]

May I ask why you need that? As it is now:

• the controllerProvider function is injectable, you can inject stuff in it in order to resolve the appropriate controller name
• the controller function itself is also injectable

[laurelnaiad]

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.

.state('myState', {
  // all the normal stuff but *not* controller property
  controllerProvider: ['appControllers', function(appControllers) { return appControllers['myState']; }]
});

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.

[nateabele]

Well, given that controller is already allowed to be a function or string, it wouldn't make any sense to restrict controllerProvider to only returning one thing or the other.

[laurelnaiad]

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.

[laurelnaiad]

Are we saying the same thing?

[nateabele]

Yeah, I was gonna say, I'm lost as to how that differs from what I just said.

[laurelnaiad]

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)

.state('myState', {
  // all the normal stuff but *not* controller property
  controllerProvider: ['appControllers', function(appControllers) { return appControllers['myState']; }]
});

is equivalent to

.state('myState', {
  // all the normal stuff but *not* controller property
  controllerProvider: { service: 'appControllers', property: 'myState' }
});

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.

[laurelnaiad]

I think there's an issue in the test -- correct me if I'm wrong:

in adrianhara@647cfd2#L1R246 :

+    it('uses the controllerProvider to select a controller name dynamically when available', inject(function ($state, $q, $httpBackend) {
+      $httpBackend.expectGET("/templates/AcmePage.html").respond("200", "some html");
+      $state.transitionTo('dynamicController', { pageType: "AcmePage" });
+      $q.flush(); 
+
+      expect(dynamicControllerName).toEqual("AcmePageController");
+    }));

Isn't this testing the test value dynamicControllerName rather than some attribute of the stateProvider/state service?

[laurelnaiad]

         result.$$controller = isFunction(view.controllerProvider) ? view.controllerProvider($stateParams) : view.controller;

I may be missing something, but this doesn't appear to be supporting injection -- it appears to be manually passing $stateParams alone.

[timkindberg]

@nateabele and @stu-salsbury you are not saying the same thing.

Currently we accept:

• string - which is the name of a controller
• function - which is the controller

@stu-salsbury and others are asking for:

• function - which returns the name of a controller
• function - which returns a controller function

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.

[laurelnaiad]

@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.

[nateabele]

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.

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.

@nateabele and @stu-salsbury you are not saying the same thing.

@timkindberg No, we are, don't worry. The unit tests will prove all. :-)

[adrianhara]

@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.

[nateabele]

@adrianhara See $templateFactory.fromProvider() for an equivalent implementation example.

[jasonkuhrt]

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.

[nateabele]

@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.

[laurelnaiad]

@adrianhara if you're stalled let me know and I can take a stab at it.

[adrianhara]

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.

[jasonkuhrt]

@nateabele @adrianhara et al great job, cheers.

[adrianhara]

Thank you @nateabele for cleaning it up!