Skip to content

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

Closed
wants to merge 35 commits into from

Conversation

adrianhara
Copy link

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
Copy link
Contributor

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

@adrianhara
Copy link
Author

@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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Contributor

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
Copy link

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
Copy link

Are we saying the same thing?

@nateabele
Copy link
Contributor

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

@laurelnaiad
Copy link

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
Copy link

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
Copy link

         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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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

@jasonkuhrt
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

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

@adrianhara
Copy link
Author

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
Copy link
Contributor

@nateabele @adrianhara et al great job, cheers.

@adrianhara
Copy link
Author

Thank you @nateabele for cleaning it up!

@christopherthielen christopherthielen modified the milestones: 0.3.0, 0.2.11 Aug 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants