Skip to content
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

Resolve can't inject value from parent resolve #73

Closed
wants to merge 27 commits into from
Closed

Conversation

ProLoser
Copy link
Member

I have a parent and child state, parent resolves some value (via ajax), then the child resolve uses that value to calculate more values.

Looks like I can't do it with the current implementation? What alternatives do I have here?

   $stateProvider.state('chromeTabs', {
      abstract: true,
      resolve:{
         tabs:  function(){
            return [{url:"fghfgh"},{}];
         }]
      }
   });

   $stateProvider.state('chromeTabs.tabContent', {
      url: '/tab/{tabId}',
      template: 'resources/templates/IframeTemplate.html',
      resolve:{
         iframeUrl: ['tabs', function(tabs){
            // tabsProvider not found
            return tabs[0].url;
         }]
      },

@coli
Copy link
Author

coli commented Apr 4, 2013

Hmm, looks like for my case, I can work around the issue by moving the 2nd resolve into a controller, since it's not firing an ajax request anyways.

@ksperling
Copy link
Contributor

Hm, I guess it would make sense to be able to do that. Currently all 'levels' of the state tree that need resolving are resolved in parallel, so to allow this type of inheritance we'd have to make that sequential instead (or be really clever and work out a dependency graph)

@coli
Copy link
Author

coli commented Apr 5, 2013

Dependency graph will be a must I think. Also angular $injector doesn't let you add context dependent stuff right now, everything is global...

@ksperling
Copy link
Contributor

$injector.invoke() has a parameter 'locals', which is where all these non-globals get passed in.

@coli
Copy link
Author

coli commented Apr 5, 2013

Nice! Also, it just occurred to me if resolve results are promises, then we can resolve top down without waiting or any additional dependency check. The syntax is not nice though.

Eg:

parent resolve: {
  tabs: function() {
    return $http();
  }
}

child resolve: {
  tabDetail: function (tabs){
    return tabs.then(function() {
      return $http();
    });
  }
}

@ksperling
Copy link
Contributor

Yeah, I definitely want to hide that from the resolve function, I think they should "feel" like any other function invoked via $injector.

@FAQinghere
Copy link

This does seem like a pretty common use case for the nested view paradigm (resolve a list for the parent, resolve a leaf for the child).

Also, it would be easy to use q to wrap the parent's resolved parameter in a promise to ensure that it be a promise.

@rynz
Copy link
Contributor

rynz commented Apr 29, 2013

@ksperling Is there a work around for this? It's something I'm having to fight with currently.

@ksperling
Copy link
Contributor

Nope, sorry no workaround, but I hope to get around to fixing this in the next week or two.

@joegaudet
Copy link

Any progress on this front?

We are currently blocked on something that would be very very nicely resolved (pardon the pun) by this fix.

In short - Do want!

:P

@joegaudet
Copy link

Bump - any chance at seeing this in the future, we've got a hellofa mess on our hands...

@ksperling anything we could do to help ?

@meenie
Copy link
Contributor

meenie commented Jul 10, 2013

+1 on this - would make things so much easier!

@rynz
Copy link
Contributor

rynz commented Jul 11, 2013

Can we have this feature also work with multiple named views, not just parents please?

Using initial example:

$stateProvider.state('chromeTabs.tabContent', {
  url: '/tab/{tabId}',
  views: {
    'list@master': {
      templateUrl: 'resources/templates/tabs.html',
      resolve: {
        tabs: function() {
          return [{url:"fghfgh"},{}];
        }
      }
    },
    'main@master': {
      templateUrl: 'resources/templates/IframeTemplate.html',
      resolve: {
        iframeUrl: ['tabs', function(tabs) {
          return tabs[0].url;
        }]
      }
    }
  }
});

@ksperling
Copy link
Contributor

@SparkiO a per-view resolve will inherit from a resolve defined at state level, but not between views themselves. So in general it will be easiest to just put everything into a single 'resolve' at state level

@joegaudet
Copy link

Any progress on this ?

@MattWalker
Copy link

@joegaudet Just created a gist that might help until the full solution is in place. I've been using this for a week or so now and it seems to do the job. https://gist.github.com/MattWalker/6106393

@xiehan
Copy link

xiehan commented Jul 29, 2013

I second the request for an update. We're hoping to release the MVP version of our project sometime between August 15~September 1st, and I would just like to know if this is going to be ready in time or if I will have to do things another way for this version.

@rynz
Copy link
Contributor

rynz commented Jul 29, 2013

This is an open source project and @ksperling isn't paid to develop ui-router. Feel free to write some code, create a pull request and contribute your own ideas / solution for this feature rather than demanding progress updates. I'm sure when it's done this issue will be updated and there is no point in nagging. ui-router provides a wealth of features already for free and if you want it done faster, donate to the developers or something and help them out and show your appreciation.

@joegaudet
Copy link

i don't think anyone was demanding anything.

On Mon, Jul 29, 2013 at 12:56 PM, Ryan Johnston notifications@github.comwrote:

This is an open source project and @ksperlinghttps://github.com/ksperlingisn't paid to develop ui-router. Feel free to write some code, create a
pull request and contribute your own ideas / solution for this feature
rather than demanding progress updates. I'm sure when it's done this issue
will be updated and there is no point in nagging. ui-router provides a
wealth of features already for free and if you want it done faster, donate
to the developers or something and help them out and show your appreciation.


Reply to this email directly or view it on GitHubhttps://github.com//issues/73#issuecomment-21746710
.

.joe out.

+1.778.994.4846

@MattWalker
Copy link

@SparkiO +1

Kudos to all involved in this project. It's made a huge difference for us.

@nateabele
Copy link
Contributor

Guys, no worries. I know this is part of a larger refactor that @ksperling is doing, so it may take a while. I wouldn't expect it imminently.

@ksperling
Copy link
Contributor

I haven't had much time to work on ui-router lately; when I started working it was in the context of a commercial project which unfortunately got put on hold, so in the last 4 months or so I've only been able to work on ui-router in my limited spare time... Anyway the $resolve service itself is written and has good test coverage, however the integration into $state may still have some quirks to iron out. So I'm just going to commit this all to a branch, and maybe one of you guys looking to use it in a real world project can contribute back any necessary fixes. Here is the branch: https://github.com/angular-ui/ui-router/tree/73-resolve

@timkindberg
Copy link
Contributor

@ksperling no worries man. You've done so much already.

@nateabele
Copy link
Contributor

@ksperling I'm using ui-router in 3 different apps that rely on this. I can get your changes integrated & tested in those apps early next week.

@ProLoser
Copy link
Member

@nateabele I converted this issue into a pull request for you. I love that trick.

  • Errors such as calling parentDependency.someMethod() which is really calling undefined.someMethod() are being gobbled up.

@ProLoser
Copy link
Member

Did some poking around and learned new stuff:

  • The inheritence works!
  • Errors inside the resolve are being gobbled up (as mentioned already)
  • Parent resolve is being re-resolved (with wrong $stateParams) again when going to a child state

I believe that the wrong $stateParams is merely a symptom of the re-resolving. The way I'm trying to switch states is using ui-sref="parent.child({key:value})" but if I do ui-sref=".child({key:value})" (ignoring the known bug) I get this:

Error: No reference point given for path '.dataset'

@ProLoser
Copy link
Member

I converted my code to the following:

ng-click="open(someId)"
...
$scope.open(someId) {
  $state.go('parent.child', { key: someId });
};

This no longer re-resolves the parent state and seems to work, but I lose my URL change I wanted.

Also, I found that if I try to go to a child state from within a resolve, then THIS resolve is re-fired

name: 'parent.child',
resolve: {
  resource: function(){
    something.then(function(){
      $state.go('parent.child.grandchild')
    })
  }
}

@ProLoser
Copy link
Member

So because I was concerned with this starting a new state change operation before the last state change op resolved (finished), I relocated the $state.go() to onEnter. However, I'm getting this annoying behavior where $stateParams and $state.params are just inconsistent and the resolvers are being fired multiple times, even when you're going deeper. It's annoying because the resolve has the proper params once, and then refiring they are missing.

@ProLoser
Copy link
Member

Okay after lots of testing and fighting, I think this is ready to merge. There are other issues I'm fighting with that should probably be new issues (like $stateParams and $state.params being convo-fucking-luted)

@alexkunin
Copy link

Another temporary solution which tries to be as easy to use (and easy to upgrade when the feature will be officially implemented) as possible: https://gist.github.com/alexkunin/6394739.

@nateabele nateabele closed this Sep 2, 2013
@nateabele
Copy link
Contributor

This is on master now. Apparently I totally screwed up my rebase the first time around, but I went back and re-did it as a cherry-pick with just the relevant commit (+ updates to make it compatible with the changes that have happened since then).

@cm325
Copy link

cm325 commented Sep 10, 2013

was this included in the 0.2.0 release?

@nateabele
Copy link
Contributor

Yes.

@meenie
Copy link
Contributor

meenie commented Sep 10, 2013

Working quite well too, thanks Nate :-).

@nateabele
Copy link
Contributor

@meenie Thank @ksperling, he wrote it. ;-)

@morgs32
Copy link

morgs32 commented Nov 8, 2013

Might have found an issue. Look like if you use "views" in the parent state and define the resolves there, inherit doesn't work.
Example:
http://plnkr.co/edit/R4ut3z?p=preview

@felix
Copy link

felix commented Nov 8, 2013

I don't think 'resolve' goes in views.

@timkindberg
Copy link
Contributor

Yeah they should be able to because each view can have a controller.

@felix
Copy link

felix commented Nov 8, 2013

Is there a benefit to having a resolve function for each view? The net-effect would be the same as a single resolve for the state, wouldn't it?

@morgs32
Copy link

morgs32 commented Nov 8, 2013

There's always a view. When you don't have a set of them then your default view is ''. There is no "resolve for the state". Am I wrong @nateabele @ksperling?

@rynz
Copy link
Contributor

rynz commented Nov 8, 2013

@morgs32 It does work you're just doing it wrong. In your example there is no way for a resolve to know which ui-view & state to inherit from.

http://plnkr.co/edit/ZcyqFlvdIu9uZGhXuPEi?p=preview is how you should be doing it.

@morgs32
Copy link

morgs32 commented Nov 8, 2013

@SparkiO are you saying that I am not supposed to be able to do what I'm trying to do? because i'm suggesting this is not your typical - straight from the wiki - example. what if i want to inherit down from a parent state that has multiple views? Anyway here's another plunkr. Let me know if I'm not being clear about why I want to use views. somewhere in the old docs it clearly stated that not having a views object only defaulted to the '' view. And that having a ui-view is the same as ui-view=''.

Can you make this one work?
http://plnkr.co/edit/drJecshVK9ILTPI2lVF7?p=preview

@nateabele
Copy link
Contributor

@morgs32 I'm really not sure what you're trying to get at, but this is definitely not defined behavior. I'm not really sure where you got the impression that resolves could be nested within view configurations, or that view configurations inherit, but neither is true and neither would make sense. States inherit. Views don't. Hope that clears everything up now.

@laurelnaiad
Copy link

IIRC, the wiki implies that resolves can be added in views using the decorator function. Maybe I'm misunderstanding.

@rynz
Copy link
Contributor

rynz commented Nov 8, 2013

@nateabele Actually resolves are nested in named view configurations.

https://github.com/angular-ui/ui-router/wiki/Multiple-Named-Views

$stateProvider
  .state('report', {
    views: {
      'filters': { ... templates, controllers, resolve, etc ... },
      'tabledata': {},
      'graph': {},
    }
  })

and I think the confusion comes from https://github.com/angular-ui/ui-router/wiki/Nested-States-%26-Nested-Views Inherited Resolved Dependencies where child states inherit resolved dependencies from parent states.

However once defining one/multiple named views it breaks this state resolve logic as it becomes named view resolve logic. What would make more sense would be resolve is always defined in state, never in named views eg:

$stateProvider
  .state('report', {
    resolve: {
      // All `named views` inherit this `state` `resolved` dependencies
      // and `named views` also inherit `resolved` dependencies from parent `states`.
    },
    views: {
      'filters': { ... templates, controllers, etc ... },
      'tabledata': {},
      'graph': {},
    }
  })

But this is not possible / does not work as currently if using named views the resolve must be defined in the named view definition.

@timkindberg
Copy link
Contributor

@nateabele yeah I think its a valid issue. Bug... request... not so clear, but I can certainly see how they expected it to work that way. How do we want to handle this? This should not have been tacked on to an already closed issue. I'd say we open a new issue and prioritize it later, perhaps putting it with 0.4.0 for now.

@rynz
Copy link
Contributor

rynz commented Nov 8, 2013

Probably best to figure out how to handle this now then create a new issue with the decision.

Personally, I see no advantage to define separate resolve dependencies within each named view as a state will not transition if any resolve dependency fails. So why not just keep resolve in the state definition.

Then Inherited Resolved Dependencies remains true where child states inherit resolved dependencies from parent states.

@laurelnaiad
Copy link

Seems to me that it would be more consistent to support resolve in views,
thus enabling apps to use decoration to automate away more boilerplate
state definition code with properties that are all and always tied to views
instead of sometimes to the state and sometimes to the view.

On Fri, Nov 8, 2013 at 2:15 PM, Ryan Johnston notifications@github.comwrote:

Probably best to figure out how to handle this now then create a new issue
with the decision.

Personally, I see no advantage to define separate resolve dependencies
within each named view as a state will not transition if any resolvedependency fails. So why not just keep
resolve in the state definition.

Then Inherited Resolved Dependencies remains true where child statesinherit resolved dependencies from parent
states.


Reply to this email directly or view it on GitHubhttps://github.com//pull/73#issuecomment-28102565
.

@timkindberg
Copy link
Contributor

@nateabele I'm confused, you said:

@morgs32 I'm really not sure what you're trying to get at, but this is definitely not defined behavior. I'm not really sure where you got the impression that resolves could be nested within view configurations

But in the code the resolve is attached to the views and I wrote it that way in the docs (obviously you can put them right on the config or in the views).

I feel like because that is the case, then this request is valid, unless I'm just being dumb...I'm just trying to figure out if we want to do it first, then we can figure out who can/will do it and when.

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.