Should get $stateChangeError transitioning to an unknown state.

[eahutchins]

$stateChangeError currently only gets broadcast when dependencies fail to resolve, it would be useful to also send it when transitionTo() is given a bogus state (both for debugging and for lazy-loading states). Alternately some new error event could get generated ($stateChangeUnknown or something).

[jeme]

Could you describe what you mean by "bogus state", because I was sure it would throw an Error if you tried to call transition to with an invalid state and not do any change at all... Which i believe is the correct behavior.

(fiddle/plunker would be great)

[eahutchins]

I'm not sure I need a plunker to show what I mean. $state.transitionTo('unknown.state') will throw like you said. What I'd like is at least the option to handle the error condition in a $stateChangeError handler instead of wrapping every transitionTo() in a try/catch so I can attempt to lazy-load the state in one centralized place. It also seems more consistent to me to have any failure to change state generate an event rather than throwing: why is failing to resolve dependencies handled differently than attempting to transition to an unknown state? Actually the nicest thing might be to throw the error if $stateChangeError has no listeners, else just broadcast the event.

[timkindberg]

@eahutchins do I smell another elegantly coded pull request?? See what I did there? Compliment followed by request. Question is, will it work on your advanced intelligence. <-- see what I did there?

[laurelnaiad]

I have some xode for this in detour. Ill see if I can extract it cleanly.

[nateabele]

👍

[laurelnaiad]

Ok, I'm really busy and it's not going to extract cleanly. $detour is under the covers so different from ui-router (starting with using a tree for state definitions) that the code is just too different, even if they match features.... tree-based definitions, merging $urlRouter into $detour (nee $state)... my code still works like ui-router but internally there are places where its quite different.

Here are some relevant places where lazy loading code lives, in case any of it's useful:

the loader config
the functions that actually bring down lazy states (for urls and state names)
where $state behaves lazily
where what was $urlRouter behaves lazily

A lot of this is in the actual laziness.

I'm still not clear on how to fit laziness into ui-router because you can't modify the states at runtime, but using an event and chaining together promises until the lazy loader has tried to get the state definition is presumably the way to go.

[eahutchins]

@timkindberg in fact I got this working the way I like yesterday, I'll put some tests together and send a pull.

@stu-salsbury why can't you modify states at runtime? I've managed to get it working (stashing out $stateProvider at bootstrap time). I've got a pending change #281 for $urlRouterProvider which always evaluates otherwise() last, which was the only problem I've run in to so far.

[laurelnaiad]

That's true, you can stash the providers. My issue was primarily with keeping the states in sync with their urlRouter counterparts (and the way 'otherwise' was handled).

[nateabele]

@eahutchins Nice work. Btw, I should have some tests put together for UrlRouterProvider sometime over the weekend.

[jeme]

What I'd like is at least the option to handle the error condition in a $stateChangeError handler instead of wrapping every transitionTo() in a try/catch so I can attempt to lazy-load the state in one centralized place.

Personally, since this framework isn't meant to do lazy loading, then that's not really a concern IMO. But if you have managed to do lazy loading without any changes to the core I can see why it annoys you.

You could however always just wrap the method your self. That way you wouldn't have to do it in more that one place.

In the case we did wan't to provide some "lazy loading" feature, I feel this scenario would be handled differently, and it's not really a stateChangeError, it would be something else...

It also seems more consistent to me to have any failure to change state generate an event rather than throwing: why is failing to resolve dependencies handled differently than attempting to transition to an unknown state?

That is quite logical to explain, resolve is considered to resolve asynchronous values, and so you can't throw errors as they would not end up with the caller.

transitionTo on the other hand is synchronous until it hits e.g. resolve... But it won't until after the state is found, so we can throw an error.

As a general rule I would always use "throw" for exception cases if I could, and only revert to events if I had to due to being in an asynchronous state.

Actually the nicest thing might be to throw the error if $stateChangeError has no listeners, else just broadcast the event.

IMO that would be worse than a configurable option, as if you hook up $stateChangeError to get notified of async errors, suddenly errors that occur synchronously would also be trapped by that, and I would rather have the error thrown at where i caused it. So i can fast and easy identify "oh so it was my call to transitionTo" that caused this.

But anyways, this is my point of view on the matter... I won't object to the change if the rest of the team is onboard.

[timkindberg]

@jeme hmmm your opposition leaves me unsure now. I understand your points. Maybe we do need a configurable option on $stateProvider, e.g. $stateProvider.handleInvalidStatesAs = "error|event";. I could see why defaulting to error is still the best solution for tracking down bugs quickly, but the option to change to event is helpful for things that @eahutchins is doing.

[eahutchins]

@jeme I had similar thoughts about the implications of handling unknown states as events rather than thrown errors, and I understand the reasoning behind throwing where possible and eventing elsewhere (which is why I asked about this before implementing anything). The throw/event dichotomy is an implementation-driven design decision, and from a user's perspective I think it's simpler to have one error handling mechanism. Wrapping transitionTo() isn't an option if it's being used internally (from ui-sref, for example). Also lazy-loading is going to be a common use case IMO, I don't see any other way to scale to really large application suites, so I do think ui-router should at least be supportive of it (and so far I've gotten our version working with surprisingly little problem).

At any rate I'm now leaning more toward a config option: $stateProvider.config(opts), I'll add a boolean to generate a $stateUndefined on undefined states and fix up the tests and add some documentation (if you don't like the method or event names let me know and I'll change them). There might be a use case for enabling and disabling the event at runtime (for example once you know everything has been lazy-loaded), so I'll support that unless anyone objects strongly.

[nateabele]

Sounds reasonable.

[eahutchins]

@nateabele should I wait for your changes to land before redoing this? I.e. how much of a pain will it be for you to merge in?